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

fix: checkbox invalid message to match figma kit #18057

Conversation

riddhybansal
Copy link
Contributor

Closes #18033

CheckboxGroup component renders the invalidText at the different position from the figma library.
With orientation="horizontal", invalidText shows at the right of the Checkboxs, and I expected it to be shown at the bottom.

Changelog

Changed

  • Seperated the container of error message from the flex container of radio buttons and hence modified the classes

Testing / Reviewing

Go to checkbox and check if invalid message is rendered correctly at bottom

Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit 7c78489
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/673db5288a2d2f000863d494
😎 Deploy Preview https://deploy-preview-18057--v11-carbon-web-components.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 Nov 13, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 7c78489
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/673db5289edd3c0008b24c6f
😎 Deploy Preview https://deploy-preview-18057--carbon-elements.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 Nov 13, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7c78489
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/673db5281332eb00089711c4
😎 Deploy Preview https://deploy-preview-18057--v11-carbon-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.

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (126c4ed) to head (7c78489).
Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #18057      +/-   ##
==========================================
+ Coverage   82.10%   82.21%   +0.11%     
==========================================
  Files         404      404              
  Lines       14107    14200      +93     
  Branches     4397     4513     +116     
==========================================
+ Hits        11582    11674      +92     
- Misses       2362     2363       +1     
  Partials      163      163              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

Hey Riddhi, It is looking good! Just one thing.
I think we should keep the invalid and readonly message inside the fieldset. If you test with VO in the current Storybook, you’ll notice that they are grouped together, which helps the screen reader user understand that the message is part of the same group label.
We can also confirm that with Raghu if needed.

@riddhybansal
Copy link
Contributor Author

Hey Riddhi, It is looking good! Just one thing. I think we should keep the invalid and readonly message inside the fieldset. If you test with VO in the current Storybook, you’ll notice that they are grouped together, which helps the screen reader user understand that the message is part of the same group label. We can also confirm that with Raghu if needed.

Oh I see what you are saying !!

@riddhybansal
Copy link
Contributor Author

riddhybansal commented Nov 17, 2024

Hey @guidari Look at this found an alternative way to fix that bug . Lemme know what do you think about it ? We can modify the test case if its a good approach to look from accessibility pov.

Copy link
Contributor

@guidari guidari left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

This does technically fix the bug, however I think it would be preferable if we just update the styles vs. changing the underlying html structure. I believe you should be able to add a inline-size (width) of 100% to the invalid or warn text when its horizontal and it will display correctly below the checkboxes.

@alisonjoseph alisonjoseph added this pull request to the merge queue Nov 20, 2024
Merged via the queue into carbon-design-system:main with commit 3772ad2 Nov 20, 2024
40 checks passed
@carbon-automation
Copy link
Contributor

Hey there! v11.71.0 was just released that references this issue/PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: invalidText of horizontal CheckboxGroup is rendered on wrong place
4 participants