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: Popover closes on change of dropdowns values #12410

Merged
merged 12 commits into from
Jan 14, 2021
Merged

fix: Popover closes on change of dropdowns values #12410

merged 12 commits into from
Jan 14, 2021

Conversation

geido
Copy link
Member

@geido geido commented Jan 11, 2021

SUMMARY

The option menuPortalTarget of the react-select component was introduced in order for the dropdowns to show fully when overlaying certain container elements. However, setting the menuPortalTarget to document.body (as originally the multi-choice dropdown did and later the single-choice) comes with several issues, such as the click events being targeted outside of the container elements, that ultimately caused this issue #12388.

This PR removes the menuPortalTarget for both single-choice and multi-choice dropdowns. In addition to that, this introduces the optional ability to force the overflow visibility of the dropdowns for cases where setting the overflow visible isn't viable on the container element (see for instance issue #12004 )

It also adjusts some excessive z-index by setting it high enough to give it precedence over the Antd Popover and other components.

Closes #12388

BEFORE

BEFORE-Num.Births.Trend.mp4

AFTER

AFTER-Num.Births.Trend.mp4

TEST PLAN

  1. Open the single and multi-choice dropdowns throughout the project
  2. Make sure the dropdowns are displaying correctly

ADDITIONAL INFORMATION

@geido
Copy link
Member Author

geido commented Jan 11, 2021

@junlincc @rusackas

@junlincc junlincc added the explore:control Related to the controls panel of Explore label Jan 11, 2021
@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #12410 (88fa5ca) into master (e47350e) will increase coverage by 4.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12410      +/-   ##
==========================================
+ Coverage   62.45%   66.79%   +4.34%     
==========================================
  Files        1015     1015              
  Lines       49537    49554      +17     
  Branches     5079     5083       +4     
==========================================
+ Hits        30937    33101    +2164     
+ Misses      18391    16323    -2068     
+ Partials      209      130      -79     
Flag Coverage Δ
cypress 51.01% <100.00%> (?)
javascript 60.75% <33.33%> (-0.02%) ⬇️
python 64.03% <ø> (+0.47%) ⬆️

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

Impacted Files Coverage Δ
.../src/explore/components/controls/SelectControl.jsx 93.45% <ø> (+6.54%) ⬆️
...rontend/src/visualizations/FilterBox/FilterBox.jsx 71.34% <ø> (+15.85%) ⬆️
...t-frontend/src/components/Select/OnPasteSelect.jsx 95.23% <100.00%> (ø)
...end/src/components/Select/SupersetStyledSelect.tsx 90.81% <100.00%> (+2.51%) ⬆️
superset/db_engine_specs/mysql.py 79.59% <0.00%> (-12.25%) ⬇️
.../src/dashboard/components/RefreshIntervalModal.tsx 87.80% <0.00%> (-1.94%) ⬇️
...-frontend/src/datasource/ChangeDatasourceModal.tsx 83.75% <0.00%> (-1.07%) ⬇️
...ews/CRUD/annotationlayers/AnnotationLayerModal.tsx 74.71% <0.00%> (-0.87%) ⬇️
...ntend/src/views/CRUD/annotation/AnnotationList.tsx 76.08% <0.00%> (-0.84%) ⬇️
...t-frontend/src/views/CRUD/welcome/SavedQueries.tsx 60.41% <0.00%> (-0.64%) ⬇️
... and 203 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 e47350e...f6446af. Read the comment docs.

@junlincc
Copy link
Member

@villebro @rusackas @ktmud need to make sure merging this PR will not create any conflict to changes in #12406

@junlincc junlincc added the rush! Requires immediate attention label Jan 11, 2021
@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 11, 2021

I found issue related to scrolling with dropdown active:
Left side - PR, before change
Go to /superset/dashboard/tabbed_dash/
Click on Region Type or select filter
Scroll screen outside the dropodown up and down
@graceguo-supercat Could you confirm we can live with this behavior as it is react-select library specific case which is not possible to fix now?
https://user-images.githubusercontent.com/25153919/104234255-6e087980-5453-11eb-82b9-dcf4da131da4.mov

@geido
Copy link
Member Author

geido commented Jan 11, 2021

For reference, this is the react-select issue related to scrolling JedWatson/react-select#4088

@junlincc junlincc requested review from ktmud and kkucharc January 11, 2021 22:19
@ktmud
Copy link
Member

ktmud commented Jan 11, 2021

Looks like a very tricky bug. I can live with the sticky menu if it doesn't happen very often. Can we add an option to SupersetStyledSelect to disable the portal wrap by default and only enable it for places where it's likely to overflow the parent container?

@junlincc junlincc added the hold:testing! On hold for testing label Jan 11, 2021
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.

Thank you for the contribution! Seems that moving menuPortalTarget higher structure may help.

I also saw scroll problem that @adam-stasiak mentioned. Unfortunately it happens in many places such as explore:
Large GIF (798x774)

@geido
Copy link
Member Author

geido commented Jan 12, 2021

I have cleaned up the things a bit. The menuPortalTarget wasn't really needed and I removed it. What is needed is only the menuPosition to be fixed for instances where setting the overflow visible on the container element isn't viable. I made that optional as @ktmud suggested by introducing a forceOverflow option so that individual cases can be handled appropriately.

When the optionforceOverflow is used, I am closing the dropdown on scroll (when the target of the scroll isn't the dropdown itself). In this way, the react-select issue (JedWatson/react-select#4088) mentioned by @adam-stasiak and @kkucharc is at least partially mitigated. It seems that the forceOverflow option is currently only required for the filters in the Dashboards. The undesired react-select behavior with scrolling should have a low impact in any case. The video below shows the new behavior.

I have also updated the PR description to better detail the problem and the chosen solution.

Sales.Dashboard.mp4

@geido geido requested a review from kkucharc January 12, 2021 20:30
@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 12, 2021

Removing data from dropdown causes modal dismiss.

Nagranie.z.ekranu.2021-01-12.o.22.51.08.mov

Moved this issue to: #12475

@geido
Copy link
Member Author

geido commented Jan 12, 2021

@adam-stasiak seems related to a different issue to me

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.

Thanks a lot for improvement!
According to the video in your comment: I understand that keeping dropdown open and sticked to the input until we click somewhere else is just impossible to achieve?

@geido
Copy link
Member Author

geido commented Jan 13, 2021

Thanks a lot for improvement!
According to the video in your comment: I understand that keeping dropdown open and sticked to the input until we click somewhere else is just impossible to achieve?

@kkucharc impossible is not as I would define it. The problem is that this is a known issue of the react-select library and it does not have an optimal solution right now. Fixing this on our side might mean coming up with hacks that are not desirable. Closing the dropdown on scroll is already some sort of hack, but I would not go beyond that really.

@kkucharc
Copy link
Contributor

@geido The video in this issue includes problem that your PR could solve. I added forceOverflow to CreatableSelect in SaveModal and it helped. Maybe you can add it in this PR so we will address the issue here as well? Otherwise we can create PR after merging this one :)

@geido
Copy link
Member Author

geido commented Jan 13, 2021

@kkucharc that's an issue affecting all the modals. I think I have a solution for that and I am going to submit a separate PR. In fact, we don't need to necessarily use the forceOverflow option to fix that problem. In general, I'd use the forceOverflow option introduced by this PR only for those cases where the overflow issue can't be solved otherwise.

EDIT: #12502

@adam-stasiak
Copy link
Contributor

Tested manually across other places in app - looks fine.

Copy link
Member

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

LGTM, tested and works great after rebase.

@ktmud ktmud merged commit c1b8a46 into apache:master Jan 14, 2021
@ktmud
Copy link
Member

ktmud commented Jan 14, 2021

@junlincc @villebro can we make sure this PR goes into v1.0, too? Currently in 1.0.0 RC2, the dropdown menu of react-select is sticky on page scroll, which is not very nice:

sticky menu

@junlincc
Copy link
Member

junlincc commented Jan 14, 2021

we are canceling rc2, kicking off rc3 to include this and a couple of others @ktmud

@villebro
Copy link
Member

@ktmud I already cancelled rc2, so I can pick this into rc3. @junlincc remember to flag all PRs you want included in 1.0 with the appropriate label so I have a checklist.

@villebro villebro added the v1.0 label Jan 14, 2021
@junlincc junlincc removed hold:testing! On hold for testing rush! Requires immediate attention labels Jan 15, 2021
villebro pushed a commit to preset-io/superset that referenced this pull request Jan 15, 2021
etr2460 pushed a commit that referenced this pull request Jan 25, 2021
* release: bump to 1.0.0 and CHANGELOG

* fix(explore): long metric name display (#12387)

* fix(explore): long metric name display

* add tooltip to control

* chore: Show datasets when search input is empty (#12391)

* chore: Fix typo “Rest” to “Reset” (#12392)

* chore: upgrade eslint, babel, and prettier (#12393)

* feat(explore): add tooltip to timepicker label (#12401)

* chore: change Datasource to Dataset in Explore ui (#12402)

* chore(explore):change dataset to datasource in ui

* modal

* Add space

* Changing it back🤦🏾‍♀️

* Chargeback

* fix: Refresh Interval Modal dropdown (#12406)

* fix(native-filters): incorrect queriesData state (#12409)

* refactor: from superset.utils.core break down date_parser (#12408)

* Fixes control panel fields styling (#12236) (#12326)

* feat: Resizable dataset and controls panels on Explore view (#12411)

* Implement resizable panels on explore view

* Optimize chart rendering while resizing

* Make dataset column narrower

Co-authored-by: Evan Rusackas <evan@preset.io>

* fix(dashboard): artefacts shown while drag and dropping deck.gl charts (#12418)

* [12181] Fix artifacts while drag and dropping deck.gl charts.

* Run prettier

* bump superset-ui packages for rolling window change (#12426)

* chore: bump superset-ui deckgl plugin (#12466)

* fix: do not show vertical scrollbar for charts in dashboard (#12478)

* fix: do not show vertical scrollbar for charts in dashboard

* Proper fix for #11419

Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>

* fix(dashboard): use datasource id from slice metadata (#12483)

* fix(timepicker): make pyparsing thread safe (#12489)

* fix: make pyparsing thread safe

* remove parenthesis for decorator

* fix (SQL Lab): disappearing results on tab switch (#12472)

* fix (SQL Lab): disappearing results on tab switch

* Remove state

* Fix test

* fix: import ZIP files that have been modified (#12425)

* fix: import ZIP files that have been modified

* Add unit test

* update changelog with rc2 entries

* fix: impose dataset ownership check on old API (#12491)

* fix: impose dataset ownership check on old API

* update UPDATING.md

* partially protect the old MVC also

* prevent metric and column add and update

* ci: remove refs/tags from docker tags on a release (#12518)

* ci: remove refs/tags from docker tags on a release

* wider head

* fix: lowercase all columns in examples (#12530)

* fix(explore): time table control panel (#12532)

* fix(explore): Add Time section back to FilterBox (#12537)

* Fixing Pinot queries for time granularities: WEEKS/MONTHS/QUARTERS/YEARS (#12536)

* fix: Select options overflowing Save chart modal on Explore view (#12522)

* Fix select options overflowing modal

* fix unit test

Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>

* Fix list filters vertical alignment (#12497)

* feat(db-engine): Add support for Apache Solr (#12403)

* [db engine] Add support for Apache Solr

* Fixing typo

* chore: rename docker image in build_docker_image.sh, docker-compose.yml and helm values.yaml (#12337)

* add rc3 changelog entries

* fix: Popover closes on change of dropdowns values (#12410)

* fix: Add MAX_SQL_ROW value to LIMIT_DROPDOWN (#12555)

* fix(viz): missing groupby and broken adhoc metrics for boxplot (#12556)

* fix: height on grid results (#12558)

* fix: case expression should not have double quotes (#12562)

* Fix 500 error when loading dashboards with slice having deleted dataset (#12535)

* add rc4 changelog entries

* Fixed typo on line 348

* Added files

Co-authored-by: Daniel Gaspar <danielvazgaspar@gmail.com>
Co-authored-by: Yongjie Zhao <yongjie.zhao@gmail.com>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Co-authored-by: Junlin Chen <junlin@preset.io>
Co-authored-by: Jesse Yang <jesse.yang@airbnb.com>
Co-authored-by: Agata Stawarz <47450693+agatapst@users.noreply.github.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: Evan Rusackas <evan@preset.io>
Co-authored-by: Kasia Kucharczyk <2536609+kkucharc@users.noreply.github.com>
Co-authored-by: Phillip Kelley-Dotson <pkelleydotson@yahoo.com>
Co-authored-by: Grace Guo <grace.guo@airbnb.com>
Co-authored-by: Beto Dealmeida <roberto@dealmeida.net>
Co-authored-by: Ville Brofeldt <ville.v.brofeldt@gmail.com>
Co-authored-by: Xiang Fu <fx19880617@gmail.com>
Co-authored-by: Ahmed Adel <github@aadel.io>
Co-authored-by: Amit Miran <47772523+amitmiran137@users.noreply.github.com>
Co-authored-by: Hugh A. Miles II <hughmil3s@gmail.com>
Co-authored-by: Shuyao Bi <shuyaob@andrew.cmu.edu>
Co-authored-by: Lyndsi Kay Williams <lyndsikaywilliams@Lyndsis-MacBook-Pro.local>
@mistercrunch mistercrunch added 🍒 1.0.0 🍒 1.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.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 explore:control Related to the controls panel of Explore size/M v1.0 🍒 1.0.0 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Explore]LONGITUDE & LATITUDE popover close after select
8 participants