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

[Component] Integrate DS Select + Multiselect #206

Merged
merged 32 commits into from
Feb 1, 2024
Merged

Conversation

meissadia
Copy link
Contributor

@meissadia meissadia commented Oct 19, 2023

Phase 1 for #197

Changes

  • Adds DS Select
  • Adds DS Multiselect.
  • Pending issues outlined here

How to test this PR

yarn vitest Select

Screenshots

Sidebar

Screenshot 2024-01-31 at 3 42 53 PM

Custom Overview

Screenshot 2024-01-31 at 3 43 00 PM

Single select Overview

Screenshot 2024-01-31 at 3 43 23 PM

Multiselect Overview

Screenshot 2024-01-31 at 3 43 37 PM

@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for cfpb-design-system-react ready!

Name Link
🔨 Latest commit 640ea2b
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/65bc05ef00bdec0007a25320
😎 Deploy Preview https://deploy-preview-206--cfpb-design-system-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@meissadia meissadia changed the title WIP: [Component] Integrate DS Select + Mulitselect WIP: [Component] Integrate DS Select + Multiselect Nov 3, 2023
@meissadia meissadia changed the title WIP: [Component] Integrate DS Select + Multiselect [Component] Integrate DS Select + Multiselect Nov 17, 2023
@meissadia meissadia marked this pull request as ready for review November 17, 2023 00:24
@meissadia
Copy link
Contributor Author

@shindigira @billhimmelsbach

In talking with @natalia-fitzgerald I've learned there are other teams experimenting with the DSR so we'd like to merge the current progress we've made on integrating the DS Select/Multiselect into DSR with the understanding that we will pick up work on the pending issues once our proposed changed to the DS Multiselect are merged and released.

Moving this PR into ready for review.

@shindigira
Copy link
Contributor

shindigira commented Nov 17, 2023

@meissadia

First error

Found this error when selecting items in the Multiselect:

Screenshot 2023-11-17 at 9 33 06 AM

EDIT: I see you are aware of it here.

Second error

Screenshot 2023-11-17 at 1 39 26 PM

Comments

@meissadia Nice work so far!

I have a couple of questions/suggestions:

  1. I think we should separate Select and Multiselect into two components. There's no overlap in markup or logic between the two.
  2. Do we want to hardwire the Tag component or create a separate component and feed it back in as a prop?
  3. Don't have this PR close issue Spike: Bring the DS Single and Multiselect into Storybook #197 yet.
  4. One important thing I don't agree with is that the Select component is now an "uncontrolled component" which somewhat goes against the React idiosyncracies.

@meissadia meissadia linked an issue Nov 17, 2023 that may be closed by this pull request
@meissadia
Copy link
Contributor Author

@shindigira

Second error

Good catch, this actually is a blocker to this component being useable, so we'll have to wait on merging these changes. This requires our DS PR to be merged, then for an update to the DS npm package to be released. That said, this PR should still be approved if things look good to avoid delays once the other dependencies are ready, we will simply wait to merge.

  1. I think we should separate Select and Multiselect into two components. There's no overlap in markup or logic between the two.

I've added the export of SelectSingle and SelectMulti so folks have the ability to directly import/use either one. I'm still a fan of having a consolidated Select for ease of use, with devs able to switch between functionalities with a single flag.

  1. Do we want to hardwire the Tag component or create a separate component and feed it back in as a prop?

We've been working toward unifying the implementation of Tag across the CFPB DS, so yes I think we'd want SelectMulti to always use the the single, approved Tag implementation, as opposed to making it customizable.

  1. Don't have this PR close issue Spike: Bring the DS Single and Multiselect into Storybook #197 yet.

👍🏾

  1. One important thing I don't agree with is that the Select component is now an "uncontrolled component" which somewhat goes against the React idiosyncracies.

There is nothing stopping implementors from passing a value prop and making SelectSingle a controlled component.

For SelectMulti, since we're leveraging the DS Multiselect which has it's own highly customized DOM element generation and event processing, we simply don't have the ability to control it.

Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Already noted the pending issues, however approving since this PR is just a phase 1.

Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Upon review, the second error that I found in my comment is breaking the components' implementation and should be addressed before approval.

@shindigira shindigira self-requested a review November 20, 2023 16:50
shindigira
shindigira previously approved these changes Nov 20, 2023
Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Approving as requested.

@shindigira shindigira self-requested a review December 1, 2023 16:45
@shindigira
Copy link
Contributor

Waiting on this PR to be merged and a new DS NPM version to be released with said PR changes.

@meissadia
Copy link
Contributor Author

meissadia commented Jan 31, 2024

@natalia-fitzgerald Updated. Would this be ready for "Verified" state?
@shindigira Many thanks for laying the ground work for this!

Sidebar

Screenshot 2024-01-30 at 5 35 11 PM

Overview

Screenshot 2024-01-30 at 5 35 19 PM

Single

screencapture-localhost-6006-2024-01-30-17_36_09

Multiple

Screenshot 2024-01-30 at 5 36 31 PM

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@meissadia
This is looking good. Just a couple of changes.

Instead of "Default" for the label text in the examples can we uss "Label"? Or do you think it would be more parallel with Text inputs to remove the label entirely?

In the DS we start with Option 1. What is Option 0?

- Update label from "Default" to "Label"
- Update option names from 0 indexed to 1 indexed
- Split options between Multi/Single
@meissadia
Copy link
Contributor Author

@natalia-fitzgerald I think we should keep the Label so end-users see that it's configurable.

  • I've updated labels from "Default" to read "Label".
  • I've updated options to start at "Option 1"
Screenshot 2024-01-31 at 9 48 37 AM

@meissadia
Copy link
Contributor Author

Updated labels to match the displayed element state:

Screenshot 2024-01-31 at 11 01 35 AM

@meissadia
Copy link
Contributor Author

  • Unit tests updated to match adjustments
  • Screenshots in PR description updated to match latest state
  • Ready for final review/merge!

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

@meissadia The documentation and content look great!

I noticed that the DSR tag is slightly taller than the DS tag. I spoke to @meissadia about this. I am not sure how we want to proceed - whether we want to push live (draft state) and then troubleshoot this (and fix in a later PR) OR wait until this is fixed. We can discuss.

CFPB Design System
Screenshot 2024-01-31 at 5 51 23 PM

CFPB Design System in React (DSR)
Screenshot 2024-01-31 at 5 51 30 PM

Copy link
Contributor

@shindigira shindigira left a comment

Choose a reason for hiding this comment

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

Tested functionally and reviewed code. It is working great!

One minor story change.

src/components/Select/Select.stories.tsx Show resolved Hide resolved
@meissadia
Copy link
Contributor Author

@meissadia The documentation and content look great!

I noticed that the DSR tag is slightly taller than the DS tag. I spoke to @meissadia about this. I am not sure how we want to proceed - whether we want to push live (draft state) and then troubleshoot this (and fix in a later PR) OR wait until this is fixed. We can discuss.

CFPB Design System Screenshot 2024-01-31 at 5 51 23 PM

CFPB Design System in React (DSR) Screenshot 2024-01-31 at 5 51 30 PM

@natalia-fitzgerald Since this issue is likely to be affecting other components that involve Icon + Text, I propose we move forward with merging the changes implemented in this PR and then addressing the icon alignment in a follow-up ticket.

I've opened #303 to track that investigation.

@meissadia meissadia dismissed natalia-fitzgerald’s stale review February 1, 2024 21:05

Opened a follow-up ticket to investigate the issue: #303

Copy link

@natalia-fitzgerald natalia-fitzgerald left a comment

Choose a reason for hiding this comment

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

Looks great @meissadia!

@meissadia meissadia merged commit 5fcb4bc into main Feb 1, 2024
7 of 9 checks passed
@meissadia meissadia deleted the 197-ds-select branch February 1, 2024 23:34
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.

Spike: Bring the DS Single and Multiselect into Storybook
4 participants