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 filter bar appearing above other elements #12480

Merged
merged 4 commits into from
Jun 28, 2017

Conversation

lukasolson
Copy link
Member

Fixes #12248.

For some reason when implementing the filter editor I chose a z-index of 20. Having such a high z-index was unnecessary and actually caused other elements (such as the autocomplete in the query bar and link tooltips) be hidden behind the filter bar.

This PR updates the z-index to a lower amount so that the filter editor still shows up above visualize but other elements can show up above it.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested in browser, looks good! I had one suggestion. What do you think?

@@ -1,7 +1,7 @@
@import (reference) "~ui/styles/variables";

filter-bar {
z-index: 20 !important;
z-index: 2 !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a suggestion which is a little bit more work, but it's very straightforward and will help us avoid this problem from popping up again in the future:

  1. Let's set this to z-index: @globalDepthFilterBar !important;.
  2. On line 158 of styles/base.less let's change > kbn-top-nav { z-index: 3; } to > kbn-top-nav { z-index: @globalDepthLocalNav; }.
  3. Let's create styles/variables/depth.less.
  4. Let's import this new file into styles/variables.less like so: @import "~ui/styles/variables/depth.less";.
  5. Inside this file, we'll assign both of these variables:
/**
 * 1. The local nav contains tooltips which should pop over the filter bar.
 */
@globalDepthFilterBar: 2; /* 1 */
@globalDepthLocalNav: 3; /* 1 */

This way the relationship of these two depths will be really obvious and if someone reintroduces this bug by changing their values, the cause should be readily apparent.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 b02b452

I added the new file to the variables.less file seeing as how both of those files were already including it... Let me know if you think it'd be better left out.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Fantastic!

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Fix looks good, but I was confused about one thing.

* 1. The local nav contains tooltips which should pop over the filter bar.
*/
@globalDepthFilterBar: 2; /* 1 */
@globalDepthLocalNav: 3; /* 1 */
Copy link
Contributor

Choose a reason for hiding this comment

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

@cjcenizal I'm a bit confused by the term "global" here. z-index isn't a global setting, it depends on the local stacking context. I'm a little worried these variable names and their location in a single global file could mislead someone about their relationship. For instance, given two variables with the values 40 and 2 in this file, depending on the stacking context of the elements they're applied to the 40 might actually appear below the 2.

Copy link
Contributor

@cjcenizal cjcenizal Jun 27, 2017

Choose a reason for hiding this comment

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

Ahhh, shoot you're right. I was thinking they had fixed positioning for some reason.

I think we still need to group these variables together because they make more sense when viewed in the same context. But the names definitely don't make sense.

I think the long-term solution is to:

  1. Move the filter_bar directory into the kbn_top_nav directory, because the two are tightly coupled for most of our apps.
  2. Import the filter_bar into the kbn_top_nav instead of in autoload/modules.js.
  3. Create a kbn_top_nav/kbn_top_nav.less file which contains these variables and the relevant kbn_top_nav styles from base.less.
  4. Rename these variables to just be filterBarDepth and localNavDepth.

It's a little bit more work, but it will be an improvement to our module organization.

Copy link
Contributor

Choose a reason for hiding this comment

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

Move the filter_bar directory into the kbn_top_nav directory, because the two are tightly coupled for most of our apps.

I don't think this is true, at least from a JS perspective. The filter_bar is a directive that can be used anywhere, not just in the kbn_top_nav. We definitely don't want to couple it to the kbn_top_nav, because each app needs the ability to conditionally render that directive depending on which query language is selected once this PR is merged. The query bar code itself doesn't even live in the kbn_top_nav, it's just in the transclude slot.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then let's just rename the variables to filterBarDepth and localNavDepth.

@lukasolson
Copy link
Member Author

I've also switched the z-index to 1 instead, because after looking more into it, it just needs to be higher than 0. Should be ready for another quick look.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Sweeeet

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM

@lukasolson lukasolson merged commit 1bea30d into elastic:master Jun 28, 2017
lukasolson added a commit that referenced this pull request Jun 28, 2017
* Fix filter bar appearing above other elements

* Make z-index relationships clearer

* Rename variables appropriately

* Use a lower z-index
lukasolson added a commit that referenced this pull request Jun 28, 2017
* Fix filter bar appearing above other elements

* Make z-index relationships clearer

* Rename variables appropriately

* Use a lower z-index
@lukasolson lukasolson deleted the fix-filter-bar-z-index branch March 27, 2018 21:10
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