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(link): exclude button elements from no-href styling #4709

Merged
merged 9 commits into from
Dec 5, 2019
Merged

fix(link): exclude button elements from no-href styling #4709

merged 9 commits into from
Dec 5, 2019

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Nov 18, 2019

Addresses issue raised in this comment: #4705 (comment)

There are situations, like with BreadcrumbItem, where bx--link class is added to an element.
If the child of BreadcrumbItem is a button, for example, then that button will get bx--link applied to it, and since the button won't have an href, it will get :disabled styling, including no pointer events. But that's problematic, as shown in #4705 (comment), because :disabled styling is being applied to an interactive button.

This PR proposes a small fix to exclude buttons from this bx--link:not([href]) rule block.

Changelog

Changed

  • exclude button elements from bx--link:not([href]) styling

Testing / Reviewing

A couple ways to test this...

  • add a Button child to a BreadcrumbItem & see that it no longer has disabled styling and no pointer events
  • add bx--link class to a Button component in the React storybook deployment for this PR (make sure you are adding the class to a button element)

@jendowns jendowns requested a review from a team as a code owner November 18, 2019 20:27
@ghost ghost requested review from asudoh and dakahn November 18, 2019 20:27
@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for the-carbon-components ready!

Built with commit 8027d08

https://deploy-preview-4709--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for carbon-components-react ready!

Built with commit 8027d08

https://deploy-preview-4709--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for carbon-elements ready!

Built with commit 8027d08

https://deploy-preview-4709--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for carbon-elements ready!

Built with commit 76a50d5

https://deploy-preview-4709--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for carbon-components-react ready!

Built with commit 76a50d5

https://deploy-preview-4709--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 18, 2019

Deploy preview for the-carbon-components ready!

Built with commit 76a50d5

https://deploy-preview-4709--the-carbon-components.netlify.com

@asudoh
Copy link
Contributor

asudoh commented Nov 18, 2019

@carbon-design-system/design Do we allow a breadcrumb item being a <button>? Thanks!

@jendowns jendowns requested a review from a team as a code owner November 19, 2019 17:25
@ghost ghost requested a review from emyarod November 19, 2019 17:25
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

should link visual styles be contained in a separate mixin instead? then rather than applying .bx--link to all breadcrumb items, we can modify the breadcrumb stylesheet to include the link styles mixin (which would not include styles based on href) on a class like .bx--breadcrumb-link or something instead

@jendowns
Copy link
Contributor Author

Thanks @emyarod! I can definitely see that being an effective approach. With the bx--link or a mixin approach, I just think the key is to make sure not to add disabled state classes to all elements just because href attribute is missing -- it definitely makes sense for anchors though!

Another thing -- until now, the component has had any child of BreadcrumbItem get that bx--link for some time -- and depending on implementation, it could theoretically be the only class getting applied to a child of BreadcrumbItem. So someone may be relying on it? 🤔 Not sure, just a thought

@joshblack joshblack merged commit fd79e78 into carbon-design-system:master Dec 5, 2019
@jendowns jendowns deleted the link-button-no-href branch December 5, 2019 15:20
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.

None yet

5 participants