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

Styling: Checkbox States (Error/Warning/Success) #1889

Merged
merged 9 commits into from
Jan 23, 2024

Conversation

shindigira
Copy link
Contributor

@shindigira shindigira commented Jan 22, 2024

closes #1883

Changes

  • Carried over the error/warning/success state styling from TextInput to Checkboxes
  • Updated the Checkbox stories to show the new styling

How to Test

Screenshot(s)

Added states

Before After
Screenshot 2024-01-22 at 12 00 47 PM Screenshot 2024-01-22 at 12 18 31 PM

Changes:

- Carried over the error/warning/success state styling from TextInput to Checkboxes
- Updated the Checkbox stories to show the new styling
@natalia-fitzgerald
Copy link
Contributor

@shindigira
Please present to match the Text inputs page. Sections will be "Status" and "Validation status"
Screenshot 2024-01-22 at 11 42 33 AM

Copy link

netlify bot commented Jan 22, 2024

Thanks for the improvements! Browse a preview of your changes using the link below.

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

Copy link

netlify bot commented Jan 22, 2024

Thanks for the improvements! Browse a preview of your changes using the link below.

Name Link
🔨 Latest commit 2fb4017
🔍 Latest deploy log https://app.netlify.com/sites/cfpb-design-system/deploys/65aea07ce4a18a0008f83b22
😎 Deploy Preview https://deploy-preview-1889--cfpb-design-system.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.

Copy link

netlify bot commented Jan 22, 2024

Thanks for the improvements! Browse a preview of your changes using the link below.

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

Copy link
Contributor

@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
I would like to show only the default states for Success, Warning, and Error.

  • Change "Variation status" (h3) to "Validation status" (h4)
  • Change "Validation status" to an H4 section
  • Show only Default state for Success, Warning, Error
    • Delete Focus state from live code example
  • Define success Default as Green (currently shows the Default checkbox style - Gray)
  • For text inputs we use outline to accomplish the 1px border. It looks like in this implementation we're using box shadow. Should we be consistent?

Screenshot 2024-01-22 at 12 17 59 PM

@shindigira
Copy link
Contributor Author

@shindigira Please present to match the Text inputs page. Sections will be "Status" and "Validation status" Screenshot 2024-01-22 at 11 42 33 AM

Updated as such! Try the preview link now.

@shindigira
Copy link
Contributor Author

shindigira commented Jan 22, 2024

@shindigira I would like to show only the default states for Success, Warning, and Error.

  • Change "Variation status" (h3) to "Validation status" (h4)

  • Change "Validation status" to an H4 section

  • Show only Default state for Success, Warning, Error

    • Delete Focus state from live code example
  • Define success Default as Green (currently shows the Default checkbox style - Gray)

  • For text inputs we use outline to accomplish the 1px border. It looks like in this implementation we're using box shadow. Should we be consistent?

Please check again. The preview link's content was stale; the focus content should not be there.

I am keeping the standard 1px border and 1px box-shadow; same as in TextInputs.

Copy link
Contributor

@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
I show that Text inputs uses outline instead of box shadow.
Screenshot 2024-01-22 at 1 02 17 PM

It looks like there was an inadvertent change to the "Standard checkbox with helper text" live code example. Instead of "Checkbox label (This is optional helper text)" it is showing "Success". Can you correct?

In current PR:
Screenshot 2024-01-22 at 1 05 44 PM

Currently live on page (correct):
Screenshot 2024-01-22 at 1 05 31 PM

@shindigira
Copy link
Contributor Author

@shindigira I show that Text inputs uses outline instead of box shadow.
It looks like there was an inadvertent change to the "Standard checkbox with helper text" live code example. Instead of "Checkbox label (This is optional helper text)" it is showing "Success". Can you correct?

Thank you for catching those issues. I have corrected the issues.

Copy link
Contributor

@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
Is it possible to define hover+focus?

  • Success: Green
  • Warning: Gold
  • Error: Red

Currently hover is still Pacific which is what I would expect since we're handling that change separately. But can we define the hover+focus as part of this change (in the image below the solid outline would be Gold)?

Screenshot 2024-01-22 at 2 14 48 PM

@shindigira
Copy link
Contributor Author

@shindigira Is it possible to define hover+focus?

  • Success: Green
  • Warning: Gold
  • Error: Red

Currently hover is still Pacific which is what I would expect since we're handling that change separately. But can we define the hover+focus as part of this change (in the image below the solid outline would be Gold)?

Screenshot 2024-01-22 at 2 14 48 PM

Resolved, please try the preview link again.

Copy link
Contributor

@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! This looks good to me.

Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@anselmbradford anselmbradford left a comment

Choose a reason for hiding this comment

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

LGTM 👍 !

@shindigira shindigira merged commit b6899c3 into main Jan 23, 2024
7 of 8 checks passed
@shindigira shindigira deleted the 1883-checkbox-default-status-boundaries branch January 23, 2024 18:03
@billhimmelsbach billhimmelsbach added the lerna-changelog/enhancement lerna label. DO NOT MODIFY label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lerna-changelog/enhancement lerna label. DO NOT MODIFY
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox - Define state styling (Success/Warning/Error status)
4 participants