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(Modal): remove z-index transition #4777

Merged
merged 5 commits into from
Nov 27, 2019

Conversation

janhassel
Copy link
Member

Closes #4690

Removes z-index transition on Modal component preventing visual and performance issues (see #4690 for more information).

Is there a reason why the z-index of the Modal needs to be -1 when hidden? I needed to re-add the transition on one state to prevent the same visual bug to appear when closing the modal. As @sabov noted though, it'd be better if z-index wouldn't be transitioned at all.

@janhassel janhassel requested a review from a team as a code owner November 26, 2019 08:34
@ghost ghost requested review from abbeyhrt and emyarod November 26, 2019 08:34
@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for the-carbon-components ready!

Built with commit 2a9f931

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

@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for carbon-elements ready!

Built with commit 2a9f931

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

@netlify
Copy link

netlify bot commented Nov 26, 2019

Deploy preview for carbon-components-react ready!

Built with commit 2a9f931

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

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.

would you also need to remove the transition on L52?

@emyarod emyarod requested a review from a team November 26, 2019 09:14
@ghost ghost requested review from aagonzales and removed request for a team November 26, 2019 09:14
@janhassel
Copy link
Member Author

@emyarod If you remove the transition from the .is-visible state as well, this visual issue appears: modal

Therefore my question is, can we have the z-index set to 9000 at all times instead of setting it to -1 when not shown?

@janhassel
Copy link
Member Author

@emyarod I pushed my proposal of the transitions with a permanent

z-index: z('modal'); // 9000

I also noticed that the visibility property was transitioned. I adjusted that as well.

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.

Oh yeah that's weird. If dev is ok with the change then it looks good to design.

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.

yeah I don't think we need that z-index transition and I wasn't able to see a reason for it from the file history, so this looks good to me

@asudoh asudoh merged commit 9422c7a into carbon-design-system:master Nov 27, 2019
@janhassel janhassel deleted the modal-z-index branch November 27, 2019 09:09
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.

Animated z-index on Modal leads to visual issues
4 participants