Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add simple transfer component #1626

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

flaminic
Copy link
Contributor

@flaminic flaminic commented Oct 30, 2024

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

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

Screenshots

Screenshot 2024-11-18 at 11 48 57

@flaminic flaminic requested a review from a team as a code owner October 30, 2024 13:06
@flaminic flaminic marked this pull request as draft October 30, 2024 13:07
@flaminic flaminic changed the title feat: add first version of simple transfer component feat: add simple transfer component Nov 18, 2024
@flaminic flaminic marked this pull request as ready for review November 18, 2024 10:53
@dhis2-bot
Copy link
Contributor

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

@dhis2-bot dhis2-bot temporarily deployed to netlify November 18, 2024 12:41 Inactive
@flaminic flaminic requested a review from a team November 18, 2024 12:58
@Topener
Copy link
Contributor

Topener commented Nov 19, 2024

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 :)

@amcgee amcgee self-requested a review November 20, 2024 08:56
@amcgee
Copy link
Member

amcgee commented Nov 20, 2024

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?

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good to me!

I think the 4 buttons could use a aria-label to describe what they're doing, using i18n so we have proper translations

>
{options.map((option, index) => {
return (
<Fragment key={option.value}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed anymore, I guess

@flaminic
Copy link
Contributor Author

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?

@Topener
Copy link
Contributor

Topener commented Nov 21, 2024

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

@Mohammer5
Copy link
Contributor

@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.

@Topener
Copy link
Contributor

Topener commented Nov 22, 2024

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).

Copy link

sonarcloud bot commented Nov 25, 2024

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

@amcgee
Copy link
Member

amcgee commented Dec 2, 2024

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
Copy link
Member

amcgee commented Dec 2, 2024

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants