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

Color palette settings button hover tootlip fix. #14693

Merged
merged 1 commit into from Apr 10, 2019
Merged

Color palette settings button hover tootlip fix. #14693

merged 1 commit into from Apr 10, 2019

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Mar 28, 2019

Description

Fixes #14685
Adding z-index fixes the issue.

How has this been tested?

Has been tested locally, more details in gif from below

Screenshots

Peek 2019-03-28 23-12

Types of changes

Had added few styles to color palette settings button in active state.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@nicolad
Copy link
Member Author

nicolad commented Apr 2, 2019

@afercia can you please confirm that this PR is fixing the issue: #14685 ?

@afercia
Copy link
Contributor

afercia commented Apr 3, 2019

@nicolad yes, it fixes it. Thanks! I don't have the powers to approve PRs though.

@gziolo
Copy link
Member

gziolo commented Apr 10, 2019

Thanks! I don't have the powers to approve PRs though.

Everyone can approve :)

@gziolo gziolo added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). User Experience (UX) [Type] Bug An existing feature does not function as intended labels Apr 10, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks like a valid fix. There is one remaining code maintainability related question to be answered before merging.

@@ -47,6 +47,8 @@ $color-palette-circle-spacing: 14px;
&.is-active {
box-shadow: inset 0 0 0 4px;
border: $border-width solid $dark-gray-400;
position: relative;
z-index: 1;
Copy link
Member

Choose a reason for hiding this comment

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

@jasmussen - should there be special handling for z-index used?

Copy link
Contributor

Choose a reason for hiding this comment

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

My impression is that's always good. But I'm honestly not confident, sometimes a z index of 1 is just to elevate a child in a context that isn't subject to the mess it can otherwise create. I know @aduth has thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

I'll leave it to him then :)

Copy link
Member

Choose a reason for hiding this comment

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

My impression has been to always use _z-index function for any z-index, with the mapping serving as a centralized authority to serve in proactively avoiding aggressively and endlessly increasing a z-index value to "combat" others.

https://github.com/WordPress/gutenberg/blob/master/assets/stylesheets/_z-index.scss

But:

  1. For a case like this, where the value is 1, perhaps it's reasonable to make exceptions
  2. In any case, we don't codify this guideline one way or the other. Whatever we decide, it ought to be written down, e.g. coding-guidelines.md document

@gziolo gziolo added this to the 5.5 (Gutenberg) milestone Apr 10, 2019
@afercia
Copy link
Contributor

afercia commented Apr 10, 2019

Note: if using z-index is not desirable, there's an alternative approach to fix this. Setting pointer-events: none on the selected button SVG should do the trick.

@gziolo
Copy link
Member

gziolo commented Apr 10, 2019

Note: if using z-index is not desirable, there's an alternative approach to fix this. Setting pointer-events: none on the selected button SVG should do the trick.

I guess z-index is perfectly fine, I just wanted to double check whether we need to use z-index() helper.

@jasmussen
Copy link
Contributor

Yep, z-index is fine. And if this PR is semi urgent or can make the cut by being merged now, feel free to and if Andrew objects to the lack of the z-index helper later on, I volunteer to fix that after the fact.

@gziolo gziolo merged commit 666eeb9 into WordPress:master Apr 10, 2019
@gziolo
Copy link
Member

gziolo commented Apr 10, 2019

@nicolad - thanks for the fix. Let's wait for feedback and take action only if necessary.

@nicolad
Copy link
Member Author

nicolad commented Apr 10, 2019

@nicolad - thanks for the fix. Let's wait for feedback and take action only if necessary.

Sure, I can add a follow-up PR based on feedback, if it will be the case.

mchowning pushed a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019
aduth pushed a commit that referenced this pull request Apr 16, 2019
aduth pushed a commit that referenced this pull request Apr 16, 2019
This was referenced Apr 17, 2019
@nicolad nicolad changed the title 14685 - color palette settings button hover tootlip fix. Color palette settings button hover tootlip fix. Sep 29, 2019
@nicolad nicolad deleted the 14685/color-settings-hover-fix branch September 29, 2019 14:47
@nicolad nicolad self-assigned this Sep 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color settings: selected color doesn't display its tooltip on hover
5 participants