-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix: breadcrumb with single character fails wcag 2.2 target size #19287
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
Changes from 3 commits
c6f4741
10c67d4
b78d1a6
a9fea1a
90ef6cb
d58a885
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -29,7 +29,7 @@ class CDSBreadcrumb extends LitElement { | |||||||||
* Specify the size of the Breadcrumb. Currently | ||||||||||
* supports the following: `sm` & `md` (default: 'md') | ||||||||||
*/ | ||||||||||
@property() | ||||||||||
@property({ type: BREADCRUMB_SIZE, reflect: true, attribute: 'size' }) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Also no need to set the |
||||||||||
size = BREADCRUMB_SIZE.MEDIUM; | ||||||||||
|
||||||||||
/** | ||||||||||
|
@@ -66,6 +66,18 @@ class CDSBreadcrumb extends LitElement { | |||||||||
`; | ||||||||||
} | ||||||||||
|
||||||||||
updated(changedProperties) { | ||||||||||
if (changedProperties.has('size')) { | ||||||||||
const items = document | ||||||||||
.querySelector(`${prefix}-breadcrumb`) | ||||||||||
?.querySelectorAll(`${prefix}-breadcrumb-item`); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
You'll want to scope the querySelector to the current instance of breadcrumb instead of |
||||||||||
items?.forEach((item) => { | ||||||||||
const link = item.querySelector(`${prefix}-breadcrumb-link`); | ||||||||||
link?.setAttribute('size', this.size); | ||||||||||
}); | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
static styles = styles; | ||||||||||
} | ||||||||||
|
||||||||||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
@warrenmblood had mentioned this issue with the Target size doesn't exist in Web Components because the margin is 8px instead of 4px in React - I wonder if we need to discuss with design to see if it's just an issue of adjusting the margins? or if this fix needs to be applied to WC too (setting the margin to 4px to align with React and then applying this same fix)?
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.
I thought to avoid changing the margins because it would affect all breadcrumbs, not just the single-character ones, which could potentially break the existing design for current users. I think it's a good idea to confirm with Design.