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: Add visibility to box for viewing menu items #12153

Merged
merged 4 commits into from
Jan 6, 2021

Conversation

maloun96
Copy link
Contributor

SUMMARY

Add visibility to filter box for viewing menu items

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE
image
AFTER
image

TEST PLAN

Go to dashboard
Set window size small that 3 dots menu can not fit into the box
See that menu is not visible at all.

ADDITIONAL INFORMATION

@codecov-io
Copy link

codecov-io commented Dec 21, 2020

Codecov Report

Merging #12153 (573ab6e) into master (d204b02) will increase coverage by 1.32%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12153      +/-   ##
==========================================
+ Coverage   65.77%   67.09%   +1.32%     
==========================================
  Files        1002     1002              
  Lines       49230    49231       +1     
  Branches     5000     4957      -43     
==========================================
+ Hits        32379    33032     +653     
+ Misses      16699    16076     -623     
+ Partials      152      123      -29     
Flag Coverage Δ
cypress 51.48% <ø> (+5.73%) ⬆️
javascript 61.25% <ø> (ø)
python 64.04% <ø> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/presto.py 74.67% <0.00%> (-8.66%) ⬇️
superset/examples/world_bank.py 97.10% <0.00%> (-2.90%) ⬇️
superset/examples/birth_names.py 96.51% <0.00%> (-2.33%) ⬇️
...et-frontend/src/SqlLab/components/LimitControl.tsx 89.36% <0.00%> (-1.95%) ⬇️
superset/models/core.py 88.58% <0.00%> (-0.28%) ⬇️
superset/connectors/sqla/models.py 91.37% <0.00%> (-0.14%) ⬇️
...c/dashboard/components/nativeFilters/FilterBar.tsx 51.79% <0.00%> (+0.71%) ⬆️
...perset-frontend/src/views/CRUD/chart/ChartList.tsx 81.88% <0.00%> (+0.78%) ⬆️
...rontend/src/SqlLab/components/AceEditorWrapper.tsx 60.75% <0.00%> (+1.26%) ⬆️
...set-frontend/src/dashboard/util/getDropPosition.js 92.06% <0.00%> (+1.58%) ⬆️
... and 79 more

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 d204b02...573ab6e. Read the comment docs.

@maloun96 maloun96 closed this Dec 22, 2020
@maloun96 maloun96 reopened this Dec 22, 2020
@rusackas rusackas requested a review from kkucharc December 22, 2020 18:05
@rusackas
Copy link
Member

@kkucharc This changes something you touched just a couple weeks ago... could you quickly verify this doesn't break the changes you made? I think it was around the Filter Box "Apply" scrolling.

@junlincc
Copy link
Member

junlincc commented Jan 2, 2021

Screen Shot 2021-01-01 at 4 22 43 PM

thanks for the fix! i can't fully test it due to this error, not sure rebasing will help? not seeing this error elsewhere @maloun96

@maloun96 maloun96 closed this Jan 4, 2021
@maloun96 maloun96 reopened this Jan 4, 2021
@villebro
Copy link
Member

villebro commented Jan 4, 2021

Screen Shot 2021-01-01 at 4 22 43 PM

thanks for the fix! i can't fully test it due to this error, not sure rebasing will help?

@junlincc do you have GLOBAL_ASYNC_QUERIES feature flag turned on?

Copy link
Contributor

@kkucharc kkucharc left a comment

Choose a reason for hiding this comment

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

I just added my comment, but maybe it's better to solve this issue and find another solution for Region Filter.

@@ -23,7 +23,7 @@
background-color: @lightest;
position: relative;
padding: 16px;
overflow-y: auto;
overflow-y: visible;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this change will affect Region Filter:
overflow-y: visible (now):
Screenshot 2021-01-04 at 16 15 00

and overflow-y: auto (was):
Screenshot 2021-01-04 at 16 14 46

I changed scroll solution back here but I believe there must be some better solution here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@kkucharc I just pulled this, and confirm that what I see looks like @maloun96 's video above. While the scrolling is... weird, it seems comparable to what was there before. I think we're OK, though I hope to find a better design solution to tiny charts/filters like this, as the scrolling is goofy.

I think we're OK here, but let me know if you see any remaining issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally fine with that :) Thank you for double checking! 👍

@junlincc junlincc requested review from junlincc and rusackas January 5, 2021 00:28
Copy link
Member

@junlincc junlincc left a comment

Choose a reason for hiding this comment

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

This PR fixes the original issue but it seems introduce 2 critical issues below.
@maloun96 please take another look, thanks!

  1. can not resize filter box individually, it drags the chart near by as resizing
Screen.Recording.2021-01-04.at.10.07.25.PM.mov
  1. all other charts(not filter box)can not be resized at all.
Screen.Recording.2021-01-04.at.10.07.41.PM.mov

@maloun96 maloun96 closed this Jan 5, 2021
@maloun96 maloun96 reopened this Jan 5, 2021
@rusackas
Copy link
Member

rusackas commented Jan 5, 2021

This PR fixes the original issue but it seems introduce 2 critical issues below.
@maloun96 please take another look, thanks!

  1. can not resize filter box individually, it drags the chart near by as resizing

Screen.Recording.2021-01-04.at.10.07.25.PM.mov

  1. all other charts(not filter box)can not be resized at all.

Screen.Recording.2021-01-04.at.10.07.41.PM.mov

I believe issue #1 is due to the two charts being part of a "Column" and you're resizing that column. If I take them out of the column, they resize individually just fine.

I also see #2, but I also see it on master. Interestingly, the resize cursor on the right edge doesn't show, but if you try the bottom right hand corner, you'll see the diagonal resize cursor, and it works just fine.

Therefore, I'm approving this PR, but will hold off from merging without your thumbs up, @junlincc

@junlincc
Copy link
Member

junlincc commented Jan 5, 2021

@rusackas you are right about issue 1, my mistake!
i can also confirm issue 2 is on master. let's address it in a separate PR

@junlincc junlincc closed this Jan 5, 2021
@junlincc junlincc reopened this Jan 5, 2021
@junlincc junlincc self-requested a review January 5, 2021 08:19
@rusackas rusackas merged commit 2bf06d6 into apache:master Jan 6, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants