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(dashboard): Filters panel height #15661

Merged
merged 5 commits into from Jul 14, 2021
Merged

fix(dashboard): Filters panel height #15661

merged 5 commits into from Jul 14, 2021

Conversation

geido
Copy link
Member

@geido geido commented Jul 13, 2021 •
edited

SUMMARY

It fixes an issue with the Filters panel height not taking the full view height in "Full-screen" mode. Additionally, it fixes an issue for which the panel appeared cut at the bottom while the page was still loading.

Fixes #15347

BEFORE

123208296-ab961780-d473-11eb-8c32-dfd920683b4f

AFTER

Screen Shot 2021-07-13 at 16 34 44

TESTING INSTRUCTIONS

  1. Open a Dashboard with native filters enbabled
  2. Observe the left side panel and make sure it takes the full height when loading
  3. Click on the dots menu far right and enable the "Full-screen" mode
  4. Observe the left side panel and make sure it gets the full height

ADDITIONAL INFORMATION

@geido
Copy link
Member Author

geido commented Jul 13, 2021

@jinghua-qa @junlincc @rusackas

@geido geido added this to In progress in Native dashboard filters via automation Jul 13, 2021
@geido geido added this to In progress in Preset-QA via automation Jul 13, 2021
@michael-s-molina
Copy link
Member

This will break the filter bar height in some scenarios:

  • Introduce a scroll when it's not required
  • Mess with the filter bar height calculation when scrolling
screen-recording-2021-07-13-at-115517-am_63fYPtlw.mp4

The key to fix this is to consider the full-screen state in the offset variable:

Screen Shot 2021-07-13 at 11 57 00 AM

michael-s-molina
michael-s-molina requested changes Jul 13, 2021
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

See comment above.

@codecov
Copy link

codecov bot commented Jul 13, 2021 •
edited

Codecov Report

Merging #15661 (0ee7fd5) into master (b3e699b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #15661   +/-   ##
=======================================
  Coverage   76.97%   76.97%           
=======================================
  Files         984      984           
  Lines       51713    51714    +1     
  Branches     6977     6977           
=======================================
+ Hits        39807    39808    +1     
  Misses      11682    11682           
  Partials      224      224
Flag Coverage Δ
javascript 71.51% <100.00%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...d/components/DashboardBuilder/DashboardBuilder.tsx 90.38% <100.00%> (+0.09%) ⬆️

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 b3e699b...0ee7fd5. Read the comment docs.

@geido
Copy link
Member Author

geido commented Jul 13, 2021

Thanks for spotting these issues. Those looked like very safe changes but I was obviously underestimating them! Let me go back to this

@geido geido marked this pull request as draft July 13, 2021 16:31
@geido geido marked this pull request as ready for review July 14, 2021 13:24
michael-s-molina
michael-s-molina approved these changes Jul 14, 2021
View reviewed changes
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@geido geido merged commit a1793f3 into apache:master Jul 14, 2021
Native dashboard filters automation moved this from In progress to Done Jul 14, 2021
Preset-QA automation moved this from In progress to Done Jul 14, 2021
@geido geido deleted the fix/issue_15347_panel branch July 14, 2021 14:06
cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
@geido
fix(dashboard): Filters panel height (apache#15661)
1fb6d50 
* Fix container height

* Add min height vh

* Remove offset when standalone

* Clean up
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
@geido
fix(dashboard): Filters panel height (apache#15661)
06382d1 
* Fix container height

* Add min height vh

* Remove offset when standalone

* Clean up
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@geido
fix(dashboard): Filters panel height (apache#15661)
f6ae4a0 
* Fix container height

* Add min height vh

* Remove offset when standalone

* Clean up
@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

@michael-s-molina michael-s-molina michael-s-molina approved these changes

@villebro villebro Awaiting requested review from villebro

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS v1.3 🚢 1.3.0
Projects
No open projects
Native dashboard filters
Done
Preset-QA
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cosmetic][native filter] native filter panel is not long enough in dashboard fullscreen mode
4 participants
@geido @michael-s-molina @mistercrunch @amitmiran137