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.

Sign up for GitHub

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

Jump to bottom

fix: Menu does not appear on scroll in Dashboard #14566

Merged
merged 2 commits into from
May 11, 2021
Merged

fix: Menu does not appear on scroll in Dashboard #14566

merged 2 commits into from
May 11, 2021

Conversation

geido
Copy link
Member

@geido geido commented May 11, 2021

SUMMARY

Removes a fixed top position and uses the header as a target for the dropdown to allow scrolling with the fixed header.

Fixes: #14553

BEFORE

https://www.awesomescreenshot.com/video/3701928?key=96c5bea8a393c3d2a2bc1e443e062b54

AFTER

https://www.awesomescreenshot.com/video/3701842?key=ee9247adb7c07874950168bf0cc461b0

TEST PLAN

ADDITIONAL INFORMATION

geido
geido commented May 11, 2021
View reviewed changes
superset-frontend/stylesheets/superset.less
@@ -603,7 +603,3 @@ hr {
top: 269px !important;
}
}

.ant-dropdown.ant-dropdown-placement-bottomRight {
top: 133px !important;
Copy link
Member Author

@geido geido May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@pkdotson this was introduced by PR #14184. Not sure what side effects might have removing it, but in general it seems that the approach of forcing the positions might not be the right one in the first place.

The potential issues were also reported by @rusackas on this review comment https://github.com/apache/superset/pull/14184/files#r626190039 but were then overlooked. I would please ask you to have a look at that portion of the changes and find different solutions. I am pretty sure this is not the only issue that we might face.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the investigation!@geido @pkdotson how much refactoring left in this project?

@jinghua-qa

@geido geido changed the title fix: Menu does not appear on scroll in Dashboard edit mode fix: Menu does not appear on scroll in Dashboard May 11, 2021
@junlincc junlincc added the bash! label May 11, 2021
@junlincc
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@junlincc Ephemeral environment spinning up at http://54.149.115.240:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@junlincc
Copy link
Member

@geido thanks for the quick fix! can you test it locally with native dashboard filter on?

@rusackas rusackas added the need:qa-review Requires QA review label May 11, 2021
@rusackas
Copy link
Member

@junlincc tested locally with native filters on. LGTM

rusackas
rusackas approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@rusackas rusackas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fix and the investigation. Re-reunning unit tests that seem to have randomly repeatedly failed.

@junlincc junlincc added the rush! Requires immediate attention label May 11, 2021
@rusackas
Copy link
Member

@geido @pkdotson Re-ran the unit tests on this PR, but they're still failing. Can you investigate/fix?

@geido
Copy link
Member Author

geido commented May 11, 2021

That's fixed @rusackas

@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #14566 (e1a1c62) into master (0fe2bf1) will decrease coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head e1a1c62 differs from pull request most recent head 7073780. Consider uploading reports for the commit 7073780 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14566      +/-   ##
==========================================
- Coverage   77.40%   77.32%   -0.09%     
==========================================
  Files         958      958              
  Lines       48326    48326              
  Branches     5677     5677              
==========================================
- Hits        37405    37366      -39     
- Misses      10721    10760      +39     
  Partials      200      200
Flag Coverage Δ
javascript 72.47% <100.00%> (ø)
presto ?

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

Impacted Files Coverage Δ
...rontend/src/explore/components/optionRenderers.tsx 100.00% <ø> (ø)
superset/examples/country_map.py 26.31% <ø> (ø)
.../components/Header/HeaderActionsDropdown/index.jsx 68.42% <100.00%> (ø)
superset/db_engine_specs/presto.py 84.42% <0.00%> (-5.48%) ⬇️
superset/connectors/sqla/models.py 88.61% <0.00%> (-1.46%) ⬇️
superset/models/core.py 88.85% <0.00%> (-0.28%) ⬇️

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 0fe2bf1...7073780. Read the comment docs.

@suddjian suddjian merged commit b960843 into apache:master May 11, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@amitmiran137 amitmiran137 added v1.2 and removed v1.2 labels May 13, 2021
@junlincc junlincc added the #bug:blocking! Blocking issues with high priority label May 13, 2021
This was referenced May 13, 2021
Closed
Closed
@yousoph
Copy link
Member

yousoph commented May 13, 2021

🏷️ 2021.19

henryyeh pushed a commit to preset-io/superset that referenced this pull request May 18, 2021
@geido @henryyeh
fix: Menu does not appear on scroll in Dashboard (apache#14566)
925a05e 
* Fix menu

* Fix test

(cherry picked from commit b960843)
@junlincc junlincc removed #bug:blocking! Blocking issues with high priority rush! Requires immediate attention labels May 27, 2021
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@geido
fix: Menu does not appear on scroll in Dashboard (apache#14566)
b022d8f 
* Fix menu

* Fix test
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@geido
fix: Menu does not appear on scroll in Dashboard (apache#14566)
1077498 
* Fix menu

* Fix test
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@geido
fix: Menu does not appear on scroll in Dashboard (apache#14566)
1373016 
* Fix menu

* Fix test
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junlincc junlincc junlincc left review comments

@rusackas rusackas rusackas approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review preset:2021.19 size/XS 🚢 1.3.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Dashboard] Menu is showing at wrong position and disappears when scrolling
7 participants
@geido @junlincc @rusackas @yousoph @mistercrunch @suddjian @amitmiran137