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

Enhancement - Options to allow key value array #762

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

syedsalehinipg
Copy link
Contributor

@syedsalehinipg syedsalehinipg commented Oct 26, 2023

Changes

  • Implemented functionality to support options arrays consisting of key-value pairs

Dependencies

N/A

UI/UX

Testing notes

Example:

[ { key: "option-a", value: "Option A" }, { key: "option-b", value: "Option B" } ]

Author checklist before assigning a reviewer

  • Reviewed my own code-diff.
  • Branch has been run in docker.
  • PR assigned to me or an appropriate delegate.
  • Relevant labels added to the PR.
  • Appropriate tests have been added.
  • Lint and test workflows pass.

@syedsalehinipg syedsalehinipg added the enhancement New feature or request label Oct 26, 2023
@syedsalehinipg syedsalehinipg self-assigned this Oct 26, 2023
@dmbarry86
Copy link
Contributor

dmbarry86 commented Oct 27, 2023

@syedsalehinipg maybe Nikitha didn't mention during the handover, but the strategy we have with RUI TypeScript migration is that all new components should be written in TypeScript and existing components get migrated when they are touched so that eventually we move everything over bit by bit.

Also, where are you planning to use this Select component? In general we should be moving to the Autocomplete.

@dmbarry86
Copy link
Contributor

I would also expect some updated tests to cater for this new syntax as well as ensuring the old syntax hasn't been broken.

Copy link
Contributor

@dmbarry86 dmbarry86 left a comment

Choose a reason for hiding this comment

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

  • Migrate to TypeScript
  • Missing tests for new behaviour
  • Question over Select usage vs Autocomplete

@syedsalehinipg
Copy link
Contributor Author

@syedsalehinipg maybe Nikitha didn't mention during the handover, but the strategy we have with RUI TypeScript migration is that all new components should be written in TypeScript and existing components get migrated when they are touched so that eventually we move everything over bit by bit.

Also, where are you planning to use this Select component? In general we should be moving to the Autocomplete.

This was going to be for the Measures dropdown in the Unit Conversion. And regarding changing it to typescript I did think that was the idea but it's just the timeframe I planned to complete this in could be delayed if I get side tracked with changing components to typescript if I have to make any small change, hence why I didn't. But if you are okay if I don't meet what's planned then I am okay doing that.

@syedsalehinipg
Copy link
Contributor Author

@syedsalehinipg maybe Nikitha didn't mention during the handover, but the strategy we have with RUI TypeScript migration is that all new components should be written in TypeScript and existing components get migrated when they are touched so that eventually we move everything over bit by bit.

Also, where are you planning to use this Select component? In general we should be moving to the Autocomplete.

Should I be using the autocomplete instead? For the unit conversions I think its easier to have dropdown for a user point of view, no?

If so I can scrap this.

@dmbarry86
Copy link
Contributor

I think its easier to have dropdown for a user point of view, no?

The autocomplete is a dropdown but is also allows to search, thus a much improved UX over Select. Run Storybook and give the Autocomplete component a try.

@syedsalehinipg
Copy link
Contributor Author

I think its easier to have dropdown for a user point of view, no?

The autocomplete is a dropdown but is also allows to search, thus a much improved UX over Select. Run Storybook and give the Autocomplete component a try.

Oh yes, I see it. It is better for sure. I might have to make the same changes here too if it doesn't support key value pair options.

@syedsalehinipg syedsalehinipg merged commit ef6c4d4 into main Nov 2, 2023
3 checks passed
@syedsalehinipg syedsalehinipg deleted the enhancement/allow-key-value-options branch November 2, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants