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

Checkbox Sidemenu Updated, Checkbox States (Error/Warning/Success) #287

Merged
merged 28 commits into from
Jan 30, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Jan 23, 2024

Closes #286
Closes #285

Changes

  • Checkbox - Storybook Sidemenu Update
  • Styling: Checkbox States (Error/Warning/Success)

Screenshots (Updated 01/28/2024)

Sidemenu Updated

Before After
Screenshot 2024-01-23 at 12 52 25 PM Screenshot 2024-01-29 at 11 21 59 AM

New Checkbox Status

Screenshot 2024-01-23 at 12 54 16 PM

Copy link

netlify bot commented Jan 23, 2024

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

Name Link
🔨 Latest commit 4a3845c
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system-react/deploys/65b94438989f6c00086c9ec1
😎 Deploy Preview https://deploy-preview-287--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.

@shindigira shindigira changed the title Checkbox Sidemenu Updated Checkbox Sidemenu Updated, Checkbox States (Error/Warning/Success) Jan 23, 2024
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
This is looking great! I have a few structural changes and then we should be good to go.

As we discussed I'd like to group this component as follows:

Sidebar menu:

  • Checkboxes
    • Checkbox
      • Overview
      • Enabled
      • Hover
      • Focus
      • Selected
      • Disabled
      • Disabled/selected
      • Success
      • Warning
      • Error
      • With helper text
    • Large target area
      • Overview
      • Enabled
      • Hover
      • Focus
      • Selected
      • Disabled
      • Disabled/selected
      • With helper text

Additional changes:


  • Change "Default" to "Enabled" for the base state
  • In helper text examples change the label text to "Label"

@shindigira
Copy link
Contributor Author

@shindigira This is looking great! I have a few structural changes and then we should be good to go.

As we discussed I'd like to group this component as follows:

Sidebar menu:

  • Checkboxes

    • Checkbox

      • Overview
      • Enabled
      • Hover
      • Focus
      • Selected
      • Disabled
      • Disabled/selected
      • Success
      • Warning
      • Error
      • With helper text
    • Large target area

      • Overview
      • Enabled
      • Hover
      • Focus
      • Selected
      • Disabled
      • Disabled/selected
      • With helper text

Additional changes:


  • Change "Default" to "Enabled" for the base state
  • In helper text examples change the label text to "Label"

Updated. Please review and provide feedback.

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
Nice work! Just a few details and we should be good to go:

  • Change "Default" to "Checkbox"
  • Is it possible for the page to read "Large target area checkbox" and still have the side menu read "Large target area"? This would match the DS.

@shindigira
Copy link
Contributor Author

shindigira commented Jan 24, 2024

  • Large target area checkbox

@natalia-fitzgerald

I changed the 'Default' to Checkbox and pushed it up.

For the last part, it seems Storybook links the two. I'd have to dig-in and see if there is a workaround.

@shindigira
Copy link
Contributor Author

  • Is it possible for the page to read "Large target area checkbox" and still have the side menu read "Large target area"? This would match the DS.

@natalia-fitzgerald As discussed, we decided not to pursue this at the moment.

@shindigira
Copy link
Contributor Author

@meissadia When reviewing, will need special attention to this autodocs change.

@shindigira shindigira requested a review from meissadia January 29, 2024 19:05
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 - Great work! This looks good and move to verified.

meissadia
meissadia previously approved these changes Jan 30, 2024
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.

Awesome job finding ways to reconfigure in order to achieve the custom Overviews!

@shindigira shindigira merged commit 307f291 into main Jan 30, 2024
7 of 9 checks passed
@shindigira shindigira deleted the checkbox-radiobuttons-sidemenu-update branch January 30, 2024 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants