feat(sbb-tag, sbb-tag-group): implement native form support #3379
+262
−79
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MarioCastigliano
requested review from
kyubisation and
jeripeierSBB
as code owners
github-actions
bot
added
the
pr: peer review required
DavideMininni-Fincons
requested changes
Feb 4, 2025
jeripeierSBB
added
the
pr: lead review required
MarioCastigliano
force-pushed
the
feat/sbb-tag-group-form-support
branch
from
2cc39e1 to
f300b36
Compare
github-actions
bot
added
pr: visual review required
and removed
pr: visual review required
labels
jeripeierSBB
requested changes
Feb 12, 2025
| @@ -42,7 +42,7 @@ $active: ':active, [data-active]'; | |||
| } | |||
|
|
|||
| // Active state | |||
| :host([checked]) { | |||
| :host([aria-pressed='true']) { | |||
There was a problem hiding this comment.
I'd recommend to handle it the same way like for the checkbox by a "data-checked".
| @@ -84,18 +81,15 @@ snapshots["sbb-tag-group renders A11y tree Chrome"] = | |||
| "children": [ | |||
| { | |||
| "role": "button", | |||
| "name": "First tag", | |||
| "pressed": false | |||
| "name": "First tag" | |||
There was a problem hiding this comment.
I am worried that the pressed state is not reflected anymore. Do you know the cause?
| @@ -110,15 +104,15 @@ snapshots["sbb-tag-group renders A11y tree Firefox"] = | |||
| "name": "", | |||
| "children": [ | |||
| { | |||
| "role": "toggle button", | |||
| "role": "button", | |||
There was a problem hiding this comment.
Hmm it doesn't seem to be recognized as toggle button anymore. Could this be preserved?
| const tagTemplate = (label: string, checked = false): TemplateResult => html` | ||
| <sbb-tag ?checked=${checked} value=${label} amount="123" icon-name="pie-small"> | ||
| const tagTemplate = (label: string, checked = false, name = 'name'): TemplateResult => html` | ||
| <sbb-tag name="${name}" ?checked=${checked} value=${label} amount="123" icon-name="pie-small"> |
There was a problem hiding this comment.
nitpicky, but in non-form examples, the name doesn't make any sense. So I recommend to not default to 'name' but have it "undefined", and then use:
Suggested change
| <sbb-tag name="${name}" ?checked=${checked} value=${label} amount="123" icon-name="pie-small"> | |
| <sbb-tag name=${name || nothing} ?checked=${checked} value=${label} amount="123" icon-name="pie-small"> |
| @forceType() | ||
| @property({ reflect: true, type: Boolean }) | ||
| public accessor checked: boolean = false; | ||
| @property({ reflect: false, type: Boolean }) |
There was a problem hiding this comment.
coercing the boolean got lost during change. but you can use !!value
| public accessor checked: boolean = false; | ||
| @property({ reflect: false, type: Boolean }) | ||
| public set checked(value: boolean) { | ||
| this._checked = value; |
There was a problem hiding this comment.
maybe setting ariaPressed should come here as well? or updateformValue goes into willupdate?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Preflight Checklist
Issue
This PR Closes #3157
Pull request checklist
Please check if your PR fulfills the following requirements:
See Review Guidelines for more information what is checked during review process.
Changes
Changes in this pull request:
Browsers
I tested the build on the following browsers:
Screen readers
I tested the build on the following browsers:
Pull request type
Please check the type of change your PR introduces:
Does this introduce a breaking change?
Other information