[Component] Integrate DS Select + Multiselect #206

meissadia · 2023-10-19T16:36:45Z

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

Custom Overview

Single select Overview

Multiselect Overview

netlify · 2023-10-19T16:36:51Z

✅ 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... Use your smartphone camera to open QR code link.

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

meissadia · 2023-11-17T00:31:39Z

@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 · 2023-11-17T14:35:00Z

@meissadia

First error

Found this error when selecting items in the Multiselect:

EDIT: I see you are aware of it here.

Second error

Comments

@meissadia Nice work so far!

I have a couple of questions/suggestions:

I think we should separate Select and Multiselect into two components. There's no overlap in markup or logic between the two.
Do we want to hardwire the Tag component or create a separate component and feed it back in as a prop?
Don't have this PR close issue Spike: Bring the DS Single and Multiselect into Storybook #197 yet.
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 · 2023-11-17T19:37:44Z

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

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.

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.

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

👍🏾

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.

shindigira

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

shindigira

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

shindigira

Approving as requested.

shindigira · 2023-12-01T16:46:04Z

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

…zed Overview

meissadia · 2024-01-31T00:45:12Z

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

Sidebar

Overview

Single

Multiple

natalia-fitzgerald

@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 · 2024-01-31T16:51:32Z

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

meissadia · 2024-01-31T18:03:04Z

Updated labels to match the displayed element state:

Naming issues resolved

meissadia · 2024-01-31T22:47:09Z

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

natalia-fitzgerald

@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

CFPB Design System in React (DSR)

shindigira

Tested functionally and reviewed code. It is working great!

One minor story change.

src/components/Select/Select.stories.tsx

…sier interaction with the menu

Addressed

meissadia · 2024-02-01T21:05:16Z

@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

CFPB Design System in React (DSR)

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

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

natalia-fitzgerald

Looks great @meissadia!

meissadia changed the title ~~WIP: [Component] Integrate DS Select + Mulitselect~~ WIP: [Component] Integrate DS Select + Multiselect Nov 3, 2023

meissadia force-pushed the 197-ds-select branch from 8d216cd to a3c16c8 Compare November 7, 2023 22:33

meissadia mentioned this pull request Nov 9, 2023

Page-by-page: Shared Platform Components for CFPB Design System (in React) #224

Open

4 tasks

meissadia added 3 commits November 16, 2023 16:50

feat: [Select] Single

44670e6

feat: [Select] Multi

3014e0a

test: [Select] Add unit tests

759a799

meissadia force-pushed the 197-ds-select branch from a3c16c8 to 759a799 Compare November 16, 2023 23:50

meissadia changed the title ~~WIP: [Component] Integrate DS Select + Multiselect~~ [Component] Integrate DS Select + Multiselect Nov 17, 2023

meissadia marked this pull request as ready for review November 17, 2023 00:24

meissadia requested review from shindigira and billhimmelsbach as code owners November 17, 2023 00:24

meissadia added 2 commits November 17, 2023 10:40

fix: Move Select to Draft state

e189ac7

feat: Allow consumer to directly use SelectMulti or SelectSingle

733390b

meissadia linked an issue Nov 17, 2023 that may be closed by this pull request

Spike: Bring the DS Single and Multiselect into Storybook #197

Closed

natalia-fitzgerald approved these changes Nov 17, 2023

View reviewed changes

shindigira approved these changes Nov 17, 2023

View reviewed changes

natalia-fitzgerald previously approved these changes Nov 17, 2023

View reviewed changes

shindigira suggested changes Nov 20, 2023

View reviewed changes

Merge branch 'main' into 197-ds-select

68dcb53

shindigira self-requested a review November 20, 2023 16:50

shindigira previously approved these changes Nov 20, 2023

View reviewed changes

shindigira self-requested a review December 1, 2023 16:45

deps: Update @cfpb/* to v0.34.0

90f7fb4

meissadia dismissed stale reviews from shindigira and natalia-fitzgerald via 90f7fb4 December 8, 2023 20:36

meissadia added 4 commits January 30, 2024 17:37

[Select] Remove intro paragraph in favor of hosting it in the customi…

f105274

…zed Overview

[Select] Separate stories for Single into its own file

147bca6

[Select] Separate stories for Multiple into its own file

db2caad

[Select] Add custom Overiew

45d1d93

meissadia requested a review from natalia-fitzgerald January 31, 2024 00:45

natalia-fitzgerald previously requested changes Jan 31, 2024

View reviewed changes

meissadia added 2 commits January 31, 2024 09:48

fix: [Selects]

a632a8e

- Update label from "Default" to "Label" - Update option names from 0 indexed to 1 indexed - Split options between Multi/Single

Merge branch 'main' into 197-ds-select

c9a90c8

meissadia requested a review from natalia-fitzgerald January 31, 2024 16:51

fix: [Selects] Update labels to match the displayed state

6cdaa74

meissadia added 3 commits January 31, 2024 11:35

fix: [Selects] Update shared Overview's 'Types' heading

d7522f8

fix: [Selects] Revert story name and label for Multiple

242dfe7

fix: [Selects] Update language

110f4cb

fix: [Selects] Update unit tests

cc3f625

natalia-fitzgerald previously requested changes Feb 1, 2024

View reviewed changes

shindigira previously requested changes Feb 1, 2024

View reviewed changes

src/components/Select/Select.stories.tsx Show resolved Hide resolved

fix: [Selects] Render Multiselect story with a min height to allow ea…

640ea2b

…sier interaction with the menu

meissadia requested a review from shindigira February 1, 2024 21:00

meissadia requested a review from natalia-fitzgerald February 1, 2024 21:05

natalia-fitzgerald approved these changes Feb 1, 2024

View reviewed changes

meissadia merged commit 5fcb4bc into main Feb 1, 2024
7 of 9 checks passed

meissadia deleted the 197-ds-select branch February 1, 2024 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Component] Integrate DS Select + Multiselect #206

[Component] Integrate DS Select + Multiselect #206

meissadia commented Oct 19, 2023 •

edited

Loading

netlify bot commented Oct 19, 2023 •

edited

Loading

meissadia commented Nov 17, 2023

shindigira commented Nov 17, 2023 •

edited

Loading

meissadia commented Nov 17, 2023

Second error

shindigira left a comment

shindigira left a comment •

edited

Loading

shindigira left a comment

shindigira commented Dec 1, 2023

meissadia commented Jan 31, 2024 •

edited

Loading

natalia-fitzgerald left a comment

meissadia commented Jan 31, 2024

meissadia commented Jan 31, 2024

meissadia commented Jan 31, 2024

natalia-fitzgerald left a comment •

edited

Loading

shindigira left a comment

meissadia commented Feb 1, 2024

natalia-fitzgerald left a comment

[Component] Integrate DS Select + Multiselect #206

[Component] Integrate DS Select + Multiselect #206

Conversation

meissadia commented Oct 19, 2023 • edited Loading

Changes

How to test this PR

Screenshots

Sidebar

Custom Overview

Single select Overview

Multiselect Overview

netlify bot commented Oct 19, 2023 • edited Loading

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

meissadia commented Nov 17, 2023

shindigira commented Nov 17, 2023 • edited Loading

First error

Second error

Comments

meissadia commented Nov 17, 2023

Second error

shindigira left a comment

Choose a reason for hiding this comment

shindigira left a comment • edited Loading

Choose a reason for hiding this comment

shindigira left a comment

Choose a reason for hiding this comment

shindigira commented Dec 1, 2023

meissadia commented Jan 31, 2024 • edited Loading

Sidebar

Overview

Single

Multiple

natalia-fitzgerald left a comment

Choose a reason for hiding this comment

meissadia commented Jan 31, 2024

meissadia commented Jan 31, 2024

meissadia commented Jan 31, 2024

natalia-fitzgerald left a comment • edited Loading

Choose a reason for hiding this comment

shindigira left a comment

Choose a reason for hiding this comment

meissadia commented Feb 1, 2024

natalia-fitzgerald left a comment

Choose a reason for hiding this comment

meissadia commented Oct 19, 2023 •

edited

Loading

netlify bot commented Oct 19, 2023 •

edited

Loading

shindigira commented Nov 17, 2023 •

edited

Loading

shindigira left a comment •

edited

Loading

meissadia commented Jan 31, 2024 •

edited

Loading

natalia-fitzgerald left a comment •

edited

Loading