increase z-index on filter box to fix bleeding of filter indicator #8579

nytai · 2019-11-15T00:08:09Z

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

nytai · 2019-11-15T00:09:32Z

superset/assets/src/visualizations/FilterBox/FilterBox.css

 .filter-container .filter-badge-container + div {
-    width: 100%;
+  width: 100%;
+  z-index: 11;


looks like vscode applied some autoformatting. This is the only actual change.

I noticed your colleague was adding some .less files to standardize some variables. i've found it quite useful to keep all z indexes used in an app in a single constant file that we can then import wherever it's needed (naming this one filter-box-dropdown-layer or something like that)

If you think that's out of scope for this PR, then maybe at least add a comment saying that we should clean that up in the future?

@etr2460 Not out of scope, definitely happy to do it in this PR. Though it probably involved converting this file to a .less file. Is there some convention around using/preferring css/less/sass(?).

z-indexs can get pretty messy pretty quickly. At some point it may be fruitful to define hierarchies throughout the app. eg, modal > dropdowns > tooltips > regular divs. So we can use variables like @z-index-dropdown to abstract away the actual values.

Yeah, defining hierarchies is something i'm definitely behind. I don't know of any preference between css/less/sass (i actually don't know why we're using all three) so i'd say you should do what you think is best here

@etr2460 I was just going to jump into this thread, to say that exactly what you're proposing is part of my LESS refactor plan. A registry of z-index "layers" and what they're used for will be super handy to grow up with.

codecov-io · 2019-11-15T00:40:21Z

Codecov Report

Merging #8579 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8579      +/-   ##
==========================================
+ Coverage   65.67%   65.69%   +0.02%     
==========================================
  Files         474      474              
  Lines       23587    23587              
  Branches     2574     2574              
==========================================
+ Hits        15491    15496       +5     
+ Misses       7927     7922       -5     
  Partials      169      169

Impacted Files	Coverage Δ
.../assets/src/visualizations/FilterBox/FilterBox.jsx	`5.93% <ø> (ø)`	⬆️
superset/utils/core.py	`88.81% <0%> (+0.16%)`	⬆️
superset/dataframe.py	`94.48% <0%> (+0.78%)`	⬆️
superset/db_engine_specs/sqlite.py	`66.66% <0%> (+9.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ff6ab10...a3b6603. Read the comment docs.

etr2460

lgtm with one possible improvement (not required though)

etr2460 · 2019-11-15T03:37:18Z

superset/assets/src/visualizations/FilterBox/FilterBox.css

 .filter-container .filter-badge-container + div {
-    width: 100%;
+  width: 100%;
+  z-index: 11;


I noticed your colleague was adding some .less files to standardize some variables. i've found it quite useful to keep all z indexes used in an app in a single constant file that we can then import wherever it's needed (naming this one filter-box-dropdown-layer or something like that)

If you think that's out of scope for this PR, then maybe at least add a comment saying that we should clean that up in the future?

etr2460

still lgtm, but a couple nits that might be nice to address

etr2460 · 2019-11-16T01:47:11Z

superset/assets/src/visualizations/FilterBox/FilterBox.less

 .filter_box:hover {
-    z-index: 1000;
+  z-index: 1000;


let's pull this one out too (while we're here?)

etr2460 · 2019-11-16T01:47:57Z

superset/assets/src/dashboard/stylesheets/variables.less

    background-color: @value;
  }
  .badge-loop(@i - 1);
 }
 .badge-loop (@iterations);
+


Can we prefix this new section with a comment? something describing that z index variables go here now?

etr2460 · 2019-11-16T01:48:16Z

superset/assets/src/dashboard/stylesheets/variables.less

@@ -16,62 +16,45 @@
 * specific language governing permissions and limitations
 * under the License.
 */
-@indicator-color: #44C0FF;
+@indicator-color: #44c0ff;


not complaining, but wondering what autoformatter fixed this. prettier?

I'm pretty sure it's prettier + vscode format on save. Happy to turn it off and revert these changes, however I saw there has been discussion about getting prettier into the FE codebase, so reverting may be a wasted effort.

No need to revert! Was only curious

etr2460

still looks good! I'll merge this if it's all done

nytai · 2019-11-20T00:25:34Z

@etr2460 everything done on my end. Please merge when you get a chance, thanks!

pull-request-size bot added the size/M label Nov 15, 2019

nytai commented Nov 15, 2019

View reviewed changes

nytai marked this pull request as ready for review November 15, 2019 00:48

etr2460 approved these changes Nov 15, 2019

View reviewed changes

dpgaspar added the preset label Nov 15, 2019

nytai force-pushed the tai/filter-z-index branch from a1cc83f to 6a4bed2 Compare November 16, 2019 00:39

etr2460 approved these changes Nov 16, 2019

View reviewed changes

nytai added 3 commits November 18, 2019 13:19

increase z-index on filter box to fix bleeding of filter indicator

a0683a8

convert to less and use variables

f99123a

add comment and port value to variable

a3b6603

nytai force-pushed the tai/filter-z-index branch from 6a4bed2 to a3b6603 Compare November 18, 2019 21:19

etr2460 approved these changes Nov 19, 2019

View reviewed changes

mistercrunch merged commit 54d9154 into apache:master Nov 20, 2019

mistercrunch deleted the tai/filter-z-index branch November 20, 2019 06:55

nytai mentioned this pull request Nov 21, 2019

fix filter dropdown z-index #8623

Merged

12 tasks

mistercrunch added preset-io and removed preset labels Nov 26, 2019

dpgaspar added v0.35 and removed v0.35 labels Dec 20, 2019

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

increase z-index on filter box to fix bleeding of filter indicator #8579

increase z-index on filter box to fix bleeding of filter indicator #8579

nytai commented Nov 15, 2019 •

edited

Loading

nytai Nov 15, 2019

etr2460 Nov 15, 2019

nytai Nov 15, 2019

etr2460 Nov 15, 2019

rusackas Nov 19, 2019

codecov-io commented Nov 15, 2019 •

edited

Loading

etr2460 left a comment

etr2460 Nov 15, 2019

etr2460 left a comment

etr2460 Nov 16, 2019

etr2460 Nov 16, 2019

etr2460 Nov 16, 2019

nytai Nov 18, 2019

etr2460 Nov 18, 2019

etr2460 left a comment

nytai commented Nov 20, 2019

increase z-index on filter box to fix bleeding of filter indicator #8579

increase z-index on filter box to fix bleeding of filter indicator #8579

Conversation

nytai commented Nov 15, 2019 • edited Loading

CATEGORY

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

REVIEWERS

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 15, 2019 • edited Loading

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

nytai commented Nov 20, 2019

nytai commented Nov 15, 2019 •

edited

Loading

codecov-io commented Nov 15, 2019 •

edited

Loading