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

Store z-index values with sass, fixes overlapping issues #637

Merged
merged 5 commits into from
May 11, 2017

Conversation

paulwilde
Copy link
Contributor

This fixes #555. Here is a before screenshot for reference:

screen shot 2017-05-03 at 22 03 20

@nylen
Copy link
Member

nylen commented May 4, 2017

Note that a flat array of z-index values is not really enough here: z-index is really a tree that gains a new branch each time a new "stacking context" is created. For more info: https://github.com/Automattic/wp-calypso/blob/master/assets/stylesheets/shared/functions/_z-index.scss

Should we go ahead and adopt an approach like this? Much easier when the project is young.

@mtias mtias added General Interface Parts of the UI which don't fall neatly under other labels. Design [Type] Question Questions about the design or development of the editor. labels May 4, 2017
@jasmussen
Copy link
Contributor

Thank you for working on this. The ticket to track it is #555.

Can I help you with this branch? I can do some rebasing if need be.

@nylen

Should we go ahead and adopt an approach like this? Much easier when the project is young.

May be a good idea, though I've always found the Calypso stacking mixin to be extremely obtuse and difficult to work with. That's not to say it's not the right approach — it very well may be, and it's just me that's daft. But just in case there's a simpler approach, @aduth do you have any ideas here?

@aduth
Copy link
Member

aduth commented May 9, 2017

I've always found the Calypso stacking mixin to be extremely obtuse and difficult to work with

It's a complex solution to a complex problem, unfortunately. Nothing comes to my mind as a simpler alternative, but @nylen is right to point out that values don't always behave as you might expect depending on the context of the element, illustrated by the example referenced in his link:

https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/The_stacking_context#The_example

I'm on the fence whether this means we should consider adopting such a mixin, or be content to play z-index whack-a-mole. Or we could look to the ecosystem to see if there are other options available.

@jasmussen
Copy link
Contributor

I'm on the fence whether this means we should consider adopting such a mixin, or be content to play z-index whack-a-mole. Or we could look to the ecosystem to see if there are other options available.

Is it worth it to start with this SASS solution, and then revisit in the future if this becomes a bigger issue?

@nylen
Copy link
Member

nylen commented May 9, 2017

Is it worth it to start with this SASS solution, and then revisit in the future if this becomes a bigger issue?

I'm fine with an incremental approach here, but I'd prefer that we start with something that more closely resembles the "correct" solution. Namely, a z-index function that looks up values in a one-dimensional rather than multi-dimensional map:

z-index: z-index( '.editor-block-switcher__arrow' );

This serves two purposes to me: it's a mental reminder that "hey z-indices are complicated", and also it'll be pretty easy to refactor later if we end up needing the "full" solution.

@jasmussen jasmussen added this to the May Week 2 milestone May 10, 2017
@@ -1,5 +1,6 @@
@import 'variables';
@import 'animations';
@import 'z-index';
Copy link
Member

Choose a reason for hiding this comment

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

This is an older prototype. I doubt we need to import z-index here.

@@ -2,7 +2,7 @@
&[data-align="left"],
&[data-align="right"] {
// Without z-index, won't be clickable as "above" adjacent content
z-index: 10;
z-index: z-index( '.editor-visual-editor__block' );
Copy link
Member

Choose a reason for hiding this comment

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

In general the names in the z-index map should correspond very closely to the selectors that apply each rule. This one is not really .editor-visual-editor__block - it should be something more like this:

.editor-visual-editor__block[data-type="core/image"][data-align]

In this case it might be a good idea to clarify the name a bit:

.editor-visual-editor__block {core/image aligned left or right}

Fixes issues with overlapping blocks/image on the header.
@nylen nylen force-pushed the fix-zindex-issues branch from aa22c6a to 97160e6 Compare May 11, 2017 14:59
Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

In order to test this, I did a comparison of the build before/after the z-index function. Something like this:

npm run build
./node_modules/.bin/css-beautify -f editor/build/style.css > editor_style.css 
GIT_DIR= git diff editor_master_style.css editor_style.css

This became much easier when I backed out all of the z-index changes (there were no changes to the build); then I re-added only the minimal change necessary to fix the issue described in dcec46b.

Let's ship this and refine later if needed.

@nylen
Copy link
Member

nylen commented May 11, 2017

Also, forgot to mention above: #555 was effectively fixed by #660 so we don't need to do anything specific to that issue in this PR.

@nylen nylen merged commit c541c16 into WordPress:master May 11, 2017
@paulwilde paulwilde deleted the fix-zindex-issues branch May 11, 2017 15:34
@gwwar
Copy link
Contributor

gwwar commented Jul 7, 2017

Totally late to this, but a single dimensional map should work for most folks. Calypso just had a lot of weird contexts. If folks ever have trouble with figuring out z-index values, you may find this extension helpful: https://github.com/gwwar/z-context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block Toolbar: z-index issue
6 participants