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: explore page style fix, remove unnecessary scroll bars #12649

Merged
merged 4 commits into from
Jan 22, 2021

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Jan 21, 2021

SUMMARY

Various style fix on Explore page. Remove unnecessary scrollbars and make select dropdowns more selectable.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Snip20210120_202

After

Snip20210120_203

  1. Remove unnecessary scroll bars
  2. Remove mysterious box shadow in chart panel
  3. Add extra spacing to dropdown menus close to container bottom when opened
  4. Allow horizontal scrolls for the dropdown menus so users can see full text of all options (wanted to allow auto-width like in SQL Lab table selector (see below), but it turns out to be much harder to do on the Explore page. We should probably just use position: absolute columns once chore(explore): Save Resizable width to localStorage #12593 is merged).

We should probably also revert the metrics/columns select in AdhocMetric/Filter popover back to react-select so that the same solution can be applied to address #12640 .

TEST PLAN

Visual verification.

I recommend testing on the "Country of Citizenship" chart.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #12649 (c8b410f) into master (e4ae17d) will decrease coverage by 3.63%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12649      +/-   ##
==========================================
- Coverage   66.85%   63.22%   -3.64%     
==========================================
  Files        1018      486     -532     
  Lines       49776    29982   -19794     
  Branches     4869        0    -4869     
==========================================
- Hits        33280    18956   -14324     
+ Misses      16373    11026    -5347     
+ Partials      123        0     -123     
Flag Coverage Δ
cypress ?
javascript ?
python 63.22% <ø> (-0.80%) ⬇️

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

Impacted Files Coverage Δ
superset/db_engines/hive.py 0.00% <0.00%> (-85.72%) ⬇️
superset/sql_validators/postgres.py 50.00% <0.00%> (-50.00%) ⬇️
superset/db_engine_specs/hive.py 54.61% <0.00%> (-29.24%) ⬇️
superset/databases/commands/create.py 83.67% <0.00%> (-8.17%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/connectors/sqla/models.py 84.31% <0.00%> (-6.28%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
superset/databases/commands/test_connection.py 84.78% <0.00%> (-4.35%) ⬇️
superset/utils/celery.py 96.42% <0.00%> (-3.58%) ⬇️
superset/models/core.py 85.59% <0.00%> (-3.27%) ⬇️
... and 537 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 e4ae17d...c8b410f. Read the comment docs.

@adam-stasiak
Copy link
Contributor

adam-stasiak commented Jan 21, 2021

Tested manually - one regression.
It occurs when using world map chart you try to scroll Country Field Type to see end of phrase.
image
moved with this PR to
image

@ktmud

@junlincc junlincc added the explore Namespace | Anything related to Explore label Jan 21, 2021
@ktmud
Copy link
Member Author

ktmud commented Jan 21, 2021

@adam-stasiak This is by design, because with the default controls column width, the dropdown can easily overflow:

So instead of horizontally scrolling the menu, you will need to scroll the whole controls column.

If folks think this is a better experience, I can revert.

@ktmud
Copy link
Member Author

ktmud commented Jan 21, 2021

cc @junlincc @mihir174

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

LGTM!

Thank you for removing unnecessary scroll bars 🙏
me personally have been annoyed by the issue you fixed in 3. thank you! 🙏
4. man, i love this 'regression'! thanks for introducing it!! ♥️
@ktmud

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.

Everything seems good to me. Thanks a lot for those changes, those charts looks really nice :)

@villebro
Copy link
Member

We should probably also revert the metrics/columns select in AdhocMetric/Filter popover back to react-select so that the same solution can be applied to address #12640 .

Reverting the popover selects to react-select feels like a step backwards. Unless there are major issues with the current AntD component, I'd rather try to address any cosmetic issues in the current component which would bring us closer to the long-term stylistic goal.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Looks great!

@ktmud ktmud merged commit 57fa6d2 into apache:master Jan 22, 2021
@junlincc
Copy link
Member

that's weird, i can't see this PR from git log and not seeing the changes in master... @ktmud

@ktmud
Copy link
Member Author

ktmud commented Jan 23, 2021

that's weird, i can't see this PR from git log and not seeing the changes in master... @ktmud

It's in master as committed 14h ago:

57fa6d2

villebro pushed a commit that referenced this pull request Jan 25, 2021
* fix: various style touch on Explore page

* More style fixes

* Force 100% height for sidebars

* Fix linting
@ktmud ktmud deleted the explore-page-style-fix branch January 26, 2021 04:50
@ktmud ktmud mentioned this pull request Jan 26, 2021
6 tasks
@mistercrunch mistercrunch added 🍒 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 Namespace | Anything related to Explore size/M v1.0.1 🍒 1.0.1 🚢 1.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants