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: content switcher icon position #2290

Merged
merged 4 commits into from
Apr 12, 2019

Conversation

lee-chase
Copy link
Member

@lee-chase lee-chase commented Apr 11, 2019

Icons not positioned correctly in the content switcher.

image

Changelog

Changed

  • src/components/content-switcher/_content-switcher.scss

Testing / Reviewing

The content switcher icon should align centrally with the text.
image

See https://codepen.io/lee-chase/pen/mgmmEN with an example of fix.

@netlify
Copy link

netlify bot commented Apr 11, 2019

Deploy preview for the-carbon-components ready!

Built with commit de8f984

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

@asudoh
Copy link
Contributor

asudoh commented Apr 11, 2019

Hi @lee-chase thank you for trying to address the use case - CC @IBM/carbon-designers to make sure that icon in content switch won't be a problem. BTW comparing https://deploy-preview-2290--the-carbon-components.netlify.com/?nav=content-switcher and https://the-carbon-components.netlify.com/?nav=content-switcher there are some content misalignments - Would you want to try to fix? Thanks!

@lee-chase
Copy link
Member Author

lee-chase commented Apr 11, 2019

@asudoh Fix attempted ;-)

p.s. Assuming you were referring to the overflow.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @lee-chase for your quick fix! A couple wrt the update:

  • Do we still need text-overflow style in content switcher button?
  • Given this is a markup change, are you willing to change our React variant as well?

@lee-chase
Copy link
Member Author

Thanks @lee-chase for your quick fix! A couple wrt the update:

  • Do we still need text-overflow style in content switcher button?
  • Given this is a markup change, are you willing to change our React variant as well?

Ah yes a bit slapdash of me, hmm must read up on what TDD has to say for superfluous code but harmless code. Probably it isn't broke don't fix it.

Strictly speaking, as there's no space between a potential icon and the label you could lose the white-space: nowrap on the btn too. Not sure I'd want to guarantee browser consistency though.

React change.. I could probably manage that, but I'd feel a little dirty ;-)

Not tonight though.

@lee-chase
Copy link
Member Author

@asudoh done

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Thanks @lee-chase for the update!

@asudoh asudoh merged commit 82bbc75 into carbon-design-system:master Apr 12, 2019
@vpicone vpicone mentioned this pull request Apr 30, 2019
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.

3 participants