fix: add disabled styling to breadcrumbs component #4783
Conversation
🦋 Changeset detectedLatest commit: 3fe7a4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| .amplify-breadcrumbs__link { | ||
| background-color: var( | ||
| --amplify-components-breadcrumbs__link-disabled-color | ||
| ); | ||
| color: var(--amplify-components-breadcrumbs__link-disabled-color); | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
Is this needed here? Should be covered by lines 45-51.
There was a problem hiding this comment.
The way it's currently written the link won't be disabled if the item is disabled, so it is necessary, but I think it would make sense to change that.
There was a problem hiding this comment.
Actually think I misunderstood, so without this you'd need to add isDisabled to Breadcrumsb.Link in addition to Breadcrumsb.Item otherwise the link would still be active.
IMO it makes sense the way it is because you can also create the component like <Breadcrumbs items=... so it might be confusing if you could pass isDisabled in with items in only that case but you'd need to put it in both Link and Item if using Breadcrumbs.Items
There was a problem hiding this comment.
Is this a fix or a feature request? I remember having some discussion with @dbanksdesign about the usefulness of adding "disabled" links to Breadcrumbs; namely that they should probably just be a text element not a link with a disabled attribute. Some things to consider:
- A link with a disabled attribute doesn't disable it the way it does for a button or input, and it doesn't convey any disabled state to assistive tech. If you test a link with a disabled attribute with a screen reader, nothing is announced about it being disabled.
- Using
pointer-events: nonein CSS still allows the link to be keyboard operable.
+1, it's against web standards to disable a link. I think it's better for the customer to just render a text element if they don't want a breadcrumb item clickable. |
Description of changes
Issue #, if available
#4776
Description of how you validated changes
Checklist
yarn testpasses and tests are updated/addedsideEffectsfield updatedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.