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(ui-shell): fix onclick menu bugs #5274

Merged
merged 6 commits into from
Feb 11, 2020

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Feb 5, 2020

Closes #4718
Closes #4413

Issue 1 can be seen here: https://codesandbox.io/s/header-with-actions-and-nav-qre4k

For some reason, will-change: transform that is hardcoded on the SVG was causing issues on expansion in a few instances in UI-shell

Changelog

Changed

  • Use ChevronDown16 instead of ChevronDownGlyph

Removed

  • unset will-change property on the chevron SVG's

Testing / Reviewing

Check the CodeSandbox, inspect the chevron SVG, and remove the will-change property. See how the dropdown works properly when this is removed.

@tw15egan tw15egan requested a review from a team as a code owner February 5, 2020 19:54
@ghost ghost requested review from asudoh and emyarod February 5, 2020 19:54
@tw15egan tw15egan changed the title fix(ui-shell): fix onclick dropdown bugs fix(ui-shell): fix onclick menu bugs Feb 5, 2020
@netlify
Copy link

netlify bot commented Feb 5, 2020

Deploy preview for carbon-components-react ready!

Built with commit 366df08

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

@netlify
Copy link

netlify bot commented Feb 5, 2020

Deploy preview for carbon-elements ready!

Built with commit 366df08

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

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.

LGTM 👍 - Thanks @tw15egan!

@asudoh asudoh requested review from a team and aagonzales and removed request for a team February 5, 2020 23:48
Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

LGTM

@xxxle0
Copy link
Contributor

xxxle0 commented Feb 7, 2020

@tw15egan, i saw we have a lots of SVG have this property will-change: transform. I think we should investigate what is actual behavior of it. How do you think ?

@tw15egan
Copy link
Member Author

tw15egan commented Feb 7, 2020

@xxxle0 agreed, would like to know why this is causing issues

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.

not sure why will-change needs to be on our icons by default, but if it's not necessary we could remove it from the icon builder package instead of overriding in UI shell

@tw15egan
Copy link
Member Author

@emyarod I'm not sure why it is there by default either, @joshblack any idea where that attribute is coming from/if it is needed?

@joshblack
Copy link
Contributor

joshblack commented Feb 10, 2020

Ah yeah, it has a specific purpose for rendering correctly in Firefox 😞 Reference: https://github.com/carbon-design-system/carbon/blob/master/packages/icon-helpers/src/getAttributes.js#L15-L17

ckeditor/ckeditor5#668 (comment)

It could easily not be needed anymore, but it did come up with our icons last year.

@emyarod
Copy link
Member

emyarod commented Feb 10, 2020

it seems to be unnecessary now so we can probably remove it as well (ckeditor/ckeditor5#1053 ckeditor/ckeditor5-ui#529)? I didn't notice any blurriness when reviewing @tw15egan's PR on Firefox

@joshblack
Copy link
Contributor

@emyarod nice, then yeah can 100% remove it upstream

@tw15egan
Copy link
Member Author

@asudoh @emyarod @joshblack updated the PR to simply remove adding the will-change: transform to all SVG elements, as the reason for its addition seems to be resolved on the Firefox end.

@joshblack
Copy link
Contributor

@tw15egan it seems like the preview for icons from netlify ran into an issue 🤔

image

I believe it's related to this line: https://github.com/carbon-design-system/carbon/blob/master/packages/icons/examples/preview/index.js#L203-L204 which was assuming style was always defined and was a string, but now it's undefined and caused the preview to fial.

@tw15egan
Copy link
Member Author

@joshblack should be resolved now

@joshblack
Copy link
Contributor

🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants