ui icon indicating copy to clipboard operation
ui copied to clipboard

feat: add simple transfer component

Open flaminic opened this issue 1 year ago • 11 comments

Implements LIBS-574

Storybook demos here.


Description

Adds an accessible version of the transfer component. This component differs from the transfer component because in this new version the transfer option ui can not be customised. This choice implies that the new component was be rewritten using a select with options, providing accessibility for free and also simplifying the code. Either than that, there should be almost no difference between the old and the new component. I say almost, because the color of an option when highlighted also changed. Now we use the default color for select/options, where customisation is not even possible (unless with very weird hacks).

Checklist

  • [x] API docs are generated
  • [x] Tests were added
  • [x] Types added / updated
  • [x] Storybook demos were added

Screenshots

Screenshot 2024-11-18 at 11 48 57

flaminic avatar Oct 30 '24 13:10 flaminic

🚀 Deployed on https://pr-1626--dhis2-ui.netlify.app

dhis2-bot avatar Nov 18 '24 12:11 dhis2-bot

Great job on the docs & demos! Once merged it should automatically be deployed to the developer portal as well and show up at the web components section there.

I cannot review the rest of the PR so I won't leave a review here :)

Topener avatar Nov 19 '24 07:11 Topener

Can we replace Transfer instead of creating a new component here? Do we know of specific cases (maybe @dhis2/analytics-frontend) where we're using the removed customization options in the existing transfer?

amcgee avatar Nov 20 '24 09:11 amcgee

Can we replace Transfer instead of creating a new component here? Do we know of specific cases (maybe @dhis2/analytics-frontend) where we're using the removed customization options in the existing transfer?

@amcgee yes we can do that. I think in the meeting we discussed marking the current transfer as deprecated first, to give some time for people to move if they need to. I'm happy with both solutions. If we want to go for the second one, i won't merge but will figure out how to mark it as deprecated first, and can add it to this PR. What do you think is best?

flaminic avatar Nov 21 '24 07:11 flaminic

I agree with @amcgee here to upgrade the old one instead of introducing a new component as a replacement. Having both is more confusing for developers than a breaking change upgrade. One small addition to this page could be enough: https://developers.dhis2.org/design/help/migrating

Topener avatar Nov 21 '24 19:11 Topener

@amcgee: Can we replace Transfer instead of creating a new component here? @flaminic: in the meeting we discussed marking the current transfer as deprecated first, to give some time for people to move if they need to @Topener: I agree with @amcgee here to upgrade the old one instead of introducing a new component as a replacement.

Adding a new component replacing the old component
Benefits Devs can upgrade the library to use unrelated changes Devs will know which component they can use without having to read documentation (they do have to read docs anyway to know the differences between the old/new component)
Trade-offs Potentially confuses developers, they'll have to read documentation Devs can't upgrade the library without also upgrading all transfer use-cases

My opinion on this is: We should trust the developers to be able to read documentation if and when they're confused, rather than trying to prevent that. It could have a much worse impact - also on our teams - when being forced to do a certain change when you actually want something completely different. I'm not sure how widely the transfer component is being used (with the select component, this would be quite a severe change), but I'm certain it has some advanced use-cases.

I wouldn't be worried about devs being confused as long as we clarify things in the docs. But I'd be worried about forcing library users to be forced to do work when they want a completely different change. Especially with a proper deprecation notice, devs would know that they're using the wrong component if they chose the old one due to confusion.

Mohammer5 avatar Nov 22 '24 01:11 Mohammer5

So I rechecked the docs, and it seems there are functional changes to this component and the transfer component. But as you indicated the Transfer component will be deprecated, we'd need to write a migration guide away from the transfer component as well. In that case, an override is not needed and it can be released as-is (per this PR).

Topener avatar Nov 22 '24 05:11 Topener

Quality Gate Failed Quality Gate failed

Failed conditions
31 New issues
1 New Bugs (required ≤ 0)
C Reliability Rating on New Code (required ≥ A)
30 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

sonarqubecloud[bot] avatar Nov 25 '24 12:11 sonarqubecloud[bot]

Devs can't upgrade the library without also upgrading all transfer use-cases

I don't think this is strictly true. You should be able to upgrade the @dhis2/ui library and, if necessary, pin the version of the transfer/select libarary to the (deprecated) one before this change, with a proper deprecation notice in console and on yarn install:

Either (in .js, assuming we publish a package which wraps the pinned versions of Transfer and SingleSelect)

import { SingleSelect, Transfer } from '@dhis2/ui-compat-v10'

OR (in package.json, no additional package needed, but has more implications for dependencies of the app)

  "resolutions": {
    "@dhis2-ui/select": "^10",
    "@dhis2-ui/transfer": "^10",
  }

The advantage of this approach, in my opinion - unlike creating a component with a new name - is that it adds some complexity for existing advanced users of a component or people upgrading, but does not introduce confusion / complexity / choice to new users of the library or these components who come in after v11. If we are deprecating and removing these older components, let's not keep them around unnecessarily.

Another option would be to do the rename but to rename the existing component to, say, LegacyTransfer / LegacySingleSelect or something, indicating very clearly that it is deprecated and shouldn't be selected for new users (and we'd similarly drop this from the next major library release). That's challenging for library use-cases, though.

amcgee avatar Dec 02 '24 09:12 amcgee

@flaminic I will make a proposal about the above and comment / commit here and on the other PR.

Would you mind taking a look at the SonarCube issues here? Either fixing or marking them as accepted. Just let me know if you have any trouble working with SonarCube!

amcgee avatar Dec 02 '24 11:12 amcgee