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

[Dropdown] Updated stylings, states and stories #176

Merged
merged 30 commits into from
Sep 27, 2023
Merged

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Sep 19, 2023

Closes #173

Changes

  • [New] Uses the standard CFPB accessibility styling (dotted outline when focused) in the Options
  • [New] Utilizes the proper CFPB-style 'down' icon in the Dropdown
  • [New] error prop that causes a red border around the Dropdown
  • [New] Dropdown Storybook story ('With Error')
  • [Update] Utilize the current library's Checkbox instead of a generic checkbox
  • [Update] Removed internal useState usage in both Checkbox and Dropdown. State must come externally in a controlled manner.
  • [Update] Update the storybook stories of Checkbox and Dropdown to use external state.
  • [Update] Update TypeScript settings
  • [Update] Update the Multi-Select variation
  • [Update] Incorporate the Menu Options styling
  • [Update] Incorporate the styling from Figma

How to test this PR

  1. yarn vitest
  2. See that everything passes
  3. yarn storybook
  4. Test the Checkbox and Dropdown stories
  5. Inspect the new styling and compare with the figma and CFPB design

Screenshots

Default

Screenshot 2023-09-19 at 4 01 28 PM

Error

Screenshot 2023-09-19 at 4 01 09 PM

Multiselect with Pills and Checkboxes

Screenshot 2023-09-19 at 4 02 01 PM

Multiselect -- Menu Options (Dotted Outline While Focused)

Screenshot 2023-09-25 at 9 53 40 AM

@shindigira shindigira added the enhancement New feature or request label Sep 19, 2023
@netlify
Copy link

netlify bot commented Sep 19, 2023

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

Name Link
🔨 Latest commit 6d2bc97
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/651369b46da44f0008e00251
😎 Deploy Preview https://deploy-preview-176--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.

fix: [Dropdown] Make story names sentance case
Copy link
Contributor

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

  • Should we try to better align stories to what is in the DS?
    • Sigmund has done a great job of demonstrating what is possible with the Dropdown component but a lot of the scenarios are not represented in the DS ("With error", "Disabled", "Multi-select with checkboxes", etc). I could use some clarification on the source for all of the features that are not outlined in the DS.

src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

  • I pushed a commit to update story names to sentence case
  • I pushed a commit to update Dropdown to Dropdowns, to align with the way we've been updating the naming of Story pages

@meissadia
Copy link
Contributor

meissadia commented Sep 19, 2023

Edit (sept 26): Per discussions, the clear all button is fine.


I had also previously added a Clear All button to the selected pills section and I'm struggling to find a reference for the source (Not seeing it in the DS or in the Figma). Should this be removed?

Screenshot 2023-09-19 at 3 50 17 PM

@shindigira
Copy link
Contributor Author

shindigira commented Sep 20, 2023

@meissadia @natalia-fitzgerald

The Clear All and the Clear button were implemented previously (before this PR) in the Dropdowns components.

The good thing is those non-CFPB Design items can be disabled via a prop.

We should discuss on how far we can deviate from the CFPB Design System.

@shindigira shindigira changed the title feat: Dropdown Updated - styling, error prop, story [Dropdown] Updated styling, error prop, story Sep 22, 2023
@shindigira shindigira changed the title [Dropdown] Updated styling, error prop, story [Dropdown] Updated stylings, states and stories Sep 22, 2023
Copy link
Contributor

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

Functionality looks great! Could use some code cleanup.

  • Border for Dropdown -> Error feels a bit heavy. I think I've seen it mentioned that it should be 2px?
  • Still in discussion but we should match the DS multiselect's :focus and :hover styles as closely as possible for now

src/components/Dropdown/Dropdown.less Outdated Show resolved Hide resolved
src/components/Dropdown/Dropdown.less Outdated Show resolved Hide resolved
src/components/Checkbox/Checkbox.stories.tsx Outdated Show resolved Hide resolved
src/components/Dropdown/Dropdown.less Outdated Show resolved Hide resolved
src/components/Dropdown/Dropdown.less Outdated Show resolved Hide resolved
src/components/Dropdown/DropdownInputWithCheckbox.tsx Outdated Show resolved Hide resolved
src/components/Dropdown/DropdownInputWithCheckbox.tsx Outdated Show resolved Hide resolved
src/components/Dropdown/styles.tsx Outdated Show resolved Hide resolved
src/components/Checkbox/Checkbox.stories.tsx Outdated Show resolved Hide resolved
@natalia-fitzgerald
Copy link

natalia-fitzgerald commented Sep 22, 2023

@shindigira

  • For the Multiselect remove the "x" within the input field (and the gray box)
    • We can handle the clear function at the form level
  • For the Multiselect use the standard CFPB styling for the focus state
    • Changes to the focus state would be global and affect the rest of our CFPB custom components (this is out of scope for this effort)

There are known bugs in the CFPB Design System - Multiselect component that we should keep in mind as we build out this component. We can then contribute back to the system while we build the React component library.

Some known bugs are documented here:

https://github.com/cfpb/design-system/issues?q=is%3Aissue+is%3Aopen+multiselect
[GHE]/CFGOV/platform/issues/3291
[GHE]/CFGOV/platform/issues/3283

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.

@shindigira

  • For the Multiselect remove the "x" within the input field (and the gray box)
    • We can handle the clear function at the form level
  • For the Multiselect use the standard CFPB styling for the focus state
    • Changes to the focus state would be global and affect the rest of our CFPB custom components (this is out of scope for this effort)

There are known bugs in the CFPB Design System - Multiselect component that we should keep in mind as we build out this component. If we have ideas about how to solve these issues we can contribute back to the system while we build the React component library.

Some known bugs are documented here:

@shindigira
Copy link
Contributor Author

Functionality looks great! Could use some code cleanup.

  • Border for Dropdown -> Error feels a bit heavy. I think I've seen it mentioned that it should be 2px?
  • Still in discussion but we should match the DS multiselect's :focus and :hover styles as closely as possible for now
  • Reduced the border to 2px.

  • There is a bit of a problem react-select -- they tie the isFocused state to both using the arrow keys and onHover.

@shindigira
Copy link
Contributor Author

shindigira commented Sep 25, 2023

  • For the Multiselect remove the "x" within the input field (and the gray box)

    • We can handle the clear function at the form level
  • For the Multiselect use the standard CFPB styling for the focus state

    • Changes to the focus state would be global and affect the rest of our CFPB custom components (this is out of scope for this effort)

There are known bugs in the CFPB Design System - Multiselect component that we should keep in mind as we build out this component. If we have ideas about how to solve these issues we can contribute back to the system while we build the React component library.

Some known bugs are documented here:

@natalia-fitzgerald Thanks for pointing out the problems in the source CFPB design system.

I followed the system as best as I can, I updated the screenshots at the top for you to review.

Also, would like to discuss about the 'Clear Form' button.

@shindigira
Copy link
Contributor Author

shindigira commented Sep 25, 2023

@meissadia I removed comments, console.logs and dead code. Thanks for pointing them out.

@meissadia meissadia self-requested a review September 26, 2023 22:45
@meissadia
Copy link
Contributor

I pushed some changes to:

  • Fix the Dropdown interactions on the Overview page
  • Remove custom styles on the in-field Clear all "X"
    • Screenshot 2023-09-26 at 5 24 46 PM
  • Address some grey border overhang on the Dropdown error state
    • Before / After
    • Screenshot 2023-09-26 at 5 37 04 PM

Copy link
Contributor

@meissadia meissadia left a comment

Choose a reason for hiding this comment

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

@shindigira Please take a look at the adjustments I made to confirm you're okay with them

@shindigira shindigira merged commit f9155fe into main Sep 27, 2023
@shindigira shindigira deleted the 173-Dropdown-Update branch September 27, 2023 15:32
@shindigira
Copy link
Contributor Author

I pushed some changes to:

  • Fix the Dropdown interactions on the Overview page

  • Remove custom styles on the in-field Clear all "X"

    • Screenshot 2023-09-26 at 5 24 46 PM
  • Address some grey border overhang on the Dropdown error state

    • Before / After
    • Screenshot 2023-09-26 at 5 37 04 PM

Thanks for this!

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.

[Component] Dropdown - Updated styling, optimized state usage
3 participants