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: nativefilter dropdown on scroll #18102

Closed
wants to merge 3 commits into from

Conversation

pkdotson
Copy link
Member

@pkdotson pkdotson commented Jan 20, 2022

SUMMARY

This pr fixes an issue when user clicks on nativefilter select and when users scrolls it causes the filter dropdown to move on scroll. This pr moves the dropdown to the parent container using a ref instead of on the page.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

after

Screen.Recording.2022-01-19.at.10.19.27.PM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina
Copy link
Member

@pkdotson Can you post the before video as well?

One tip: If you leave a blank line between the after word and the video link then the video is automatically loaded in the PR description. Like this:

After:
(blank_line)
<video_link>

@geido
Copy link
Member

geido commented Jan 21, 2022

Hey @pkdotson if you can fix the CI we should spin up a testenv. This approach has been proven problematic in the past and I think we need to go deep into testing this out.

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #18102 (db04b6c) into master (5da3c45) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18102      +/-   ##
==========================================
- Coverage   66.34%   66.34%   -0.01%     
==========================================
  Files        1569     1569              
  Lines       61687    61688       +1     
  Branches     6241     6243       +2     
==========================================
- Hits        40927    40926       -1     
+ Misses      19162    19161       -1     
- Partials     1598     1601       +3     
Flag Coverage Δ
javascript 50.91% <80.00%> (-0.01%) ⬇️

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

Impacted Files Coverage Δ
.../FilterBar/CascadeFilters/CascadePopover/index.tsx 48.33% <ø> (ø)
...ilters/FilterBar/FilterControls/FilterControls.tsx 77.14% <ø> (ø)
...c/filters/components/Select/SelectFilterPlugin.tsx 69.01% <0.00%> (ø)
...nd/src/filters/components/Select/transformProps.ts 90.90% <ø> (ø)
...es/superset-ui-core/src/chart/models/ChartProps.ts 100.00% <100.00%> (ø)
superset-frontend/src/components/Select/Select.tsx 86.59% <100.00%> (ø)
...board/components/nativeFilters/FilterBar/index.tsx 82.72% <100.00%> (+0.15%) ⬆️
...d/src/SqlLab/components/SqlEditorLeftBar/index.jsx 54.28% <0.00%> (-1.27%) ⬇️
...frontend/src/SqlLab/components/SqlEditor/index.jsx 51.14% <0.00%> (-0.62%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 60.39% <0.00%> (-0.48%) ⬇️
... and 9 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 5da3c45...db04b6c. Read the comment docs.

@@ -89,6 +89,12 @@ export interface SelectProps extends PickedSelectProps {
* False by default.
* */
allowNewOptions?: boolean;
/**
Copy link
Member

Choose a reason for hiding this comment

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

This comment should be generic. When you say something like to prevent dropdown movement onscroll you are limiting its uses.

You could use the Ant Design docs for this prop:

Parent Node to which the selector should be rendered.

@geido
Copy link
Member

geido commented Jan 24, 2022

/testenv up

@github-actions
Copy link
Contributor

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

@geido
Copy link
Member

geido commented Jan 24, 2022

One contingent issue that is appearing with these changes is that sometimes the dropdown is not fully visible and you won't be able to scroll the container down either, as you can see in the video.

Unicode.Test.1.mp4

Before you could still scroll the dashboard container (right side) and the dropdown menu would move with it, basically making it visible. This change is not necessarily worst than the previous behavior just pointing it out for visibility.

@@ -271,6 +277,7 @@ const Select = ({
ariaLabel,
fetchOnlyOnSearch,
filterOption = true,
getPopupContainer,
Copy link
Member

Choose a reason for hiding this comment

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

put this on top of filterOption

@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

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.

None yet

4 participants