-
Notifications
You must be signed in to change notification settings - Fork 51
Fix Hds::Form::Label argument for with boolean values
#2863
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where the Hds::Form::Label's "for" attribute is not properly rendered when the @controlid argument is a boolean, ensuring consistent behavior for both true and false values.
- Forced the "for" attribute to be converted to a string
- Added an integration test to verify proper rendering with boolean values
- Updated the changeset patch note to reflect the changes
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| showcase/tests/integration/components/hds/form/label/index-test.js | Added integration tests to verify that the "for" attribute is rendered as a string even when @controlid is a boolean. |
| .changeset/nine-fans-cough.md | Updated changeset with patch note detailing the forced string conversion of the "for" attribute. |
Files not reviewed (2)
- packages/components/src/components/hds/form/label/index.hbs: Language not supported
- showcase/app/templates/components/form/radio.hbs: Language not supported
|
Nice! I just noticed that in the showcase, when the id is set to false it still has the ember generated id. |
📌 Summary
While working on hashicorp/vault#30555 I discovered a strange "bug" in the
Hds::Form::Label: essentially in the case of a radio group (anHds::Form::Field) where the possible values are booleanstrue/falseand the values are used also asIDfor the elements, theforHTML attribute behaves differently than theidHTML attribute:for={{true}}in the template → renders asforattribute, without valuefor={{false}}in the template → it does not render theforattributeid={{true}}in the template → renders asid="true"attributeid={{false}}in the template → renders asid="false"attributeThis means that, while for the
falsecase the association between theidof theinputand theforof the label is maintained, because the ID is generated asguidFor()by thegetElementId()function, in thetruecase this association is broken, because thegetElementId()function sees that an ID is provided (element.args.idevaluates totrue), and so it passes this argument to theidof the input, which is converted to a string, and passed to theforof the label, which is not converted to a string but simply omitted.To fix this discrepancy, I have just wrapped in double quotes the
forassignment in the handlebars template of theForm::Labelcomponent.🛠️ Detailed description
In this PR I have:
forattribute inHds::Form::Labelto be a stringHds::Form::Radio::Groupwith boolean values to showcaseHds::Form::Labelfor use case when@controlIdis a booleanHds::Form::Radio::Groupfor use case when RadioField@idis a boolean👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.