feat: add simple transfer component
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
🚀 Deployed on https://pr-1626--dhis2-ui.netlify.app
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 :)
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?
Can we replace
Transferinstead 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?
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
@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.
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).
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
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.
@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!
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
54.1% Duplication on New Code