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: [M3-7665] - RegionMultiSelect Component #10084

Merged
merged 11 commits into from
Jan 23, 2024
Merged

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Jan 19, 2024

Description πŸ“

This Allows to select multiple regions and renders the list of selected regions

Changes πŸ”„

  • RemovableSelectionsList - Added an optional prop to render custom label component.

Preview πŸ“·

Region Multi select Renders selected list
image image

How to test πŸ§ͺ

  • Run story book verify the functionality of Region Multi select component.

As an Author I have considered πŸ€”

Check all that apply

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@cpathipa cpathipa requested a review from a team as a code owner January 19, 2024 14:54
@cpathipa cpathipa requested review from bnussman-akamai and jaalah-akamai and removed request for a team January 19, 2024 14:54
@cpathipa cpathipa marked this pull request as draft January 19, 2024 14:54
@cpathipa cpathipa self-assigned this Jan 19, 2024
Copy link

github-actions bot commented Jan 19, 2024

Coverage Report: βœ…
Base Coverage: 79.95%
Current Coverage: 80.18%

@cpathipa cpathipa marked this pull request as ready for review January 19, 2024 19:07
@jaalah-akamai
Copy link
Contributor

The dark mode background colors/shading is a little weird. Do we also want to have a border under Select All and Deselect All like in the original Autocomplete example?

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for breaking this down in its own component!

The component tested in Storybook looks good πŸ‘

Left a couple questions and some comments to improve code

Also, I am not a fan of this UI pattern in general. When selecting a region, the user has no idea a list is behind the dropdown, and the checkmark next to the region is faint. There's a risk the user does not know the selection worked. I'd like to bring this to UX cause we may want to re-thing this pattern. CC @jaalah-akamai

Screenshot 2024-01-22 at 10 28 40

@cpathipa
Copy link
Contributor Author

cpathipa commented Jan 22, 2024

@jaalah-akamai @jaalah-akamai Great feedback.

  • Renamed - RenderSelectedRegionsList to SelectedRegionsList
  • Renamed - removeOption to handleRemoveOption
  • improved readability in story book variable naming - selectedRegionsIds, setSelectedRegionsIds
  • Removed debugger and commented code and described the util function.
  • Added UX concern of selected region list and checkmark selection item to our next cafe.
  • Added border under Select All and Deselect All.
  • Extract RenderOption as a Separate Component in RegionMultiSelect
  • RegionMultiSelect -Test coverage
  • Created a followup ticket M3-7675 for coverage and to make renderOption as reusable for RegionSelect and RegionMultiSelect.

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! - a couple more comments to clean things up and this should be good to go

….stories.tsx

Co-authored-by: Alban Bailly <130582365+abailly-akamai@users.noreply.github.com>
@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Jan 23, 2024
Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for re-using RegionOption, much better. I'm sure there's more we can do, but this is good for now

Copy link
Contributor

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

One day, we may want to combine RegionSelect and RegionMultiSelect into one component, but this is good for now!

@cpathipa cpathipa merged commit c432177 into linode:develop Jan 23, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add'tl Approval Needed Waiting on another approval!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants