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

refactor: Migrate react-select to Antd Select in Metrics and Filters popovers #12042

Merged
merged 5 commits into from
Dec 16, 2020

Conversation

kgabryje
Copy link
Member

SUMMARY

As a first part of #11916, I migrated react-select components in Filters and Metrics popovers to Antd. Next step will be refactoring MetricsControl and AdhocFiltersControl according to designs.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

As you can see on the before/after screenshot, there is 1 difference - in the bottom select input we no longer display placeholder that says how many options are left when there's at least 1 option selected. Antd doesn't support displaying such label.

Before:
image
After:
image

TEST PLAN

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

CC: @junlincc @villebro @adam-stasiak

@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #12042 (52690b7) into master (1afe915) will decrease coverage by 3.80%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12042      +/-   ##
==========================================
- Coverage   67.71%   63.90%   -3.81%     
==========================================
  Files         952      952              
  Lines       46686    46707      +21     
  Branches     4577     4586       +9     
==========================================
- Hits        31615    29850    -1765     
- Misses      14958    16673    +1715     
- Partials      113      184      +71     
Flag Coverage Δ
cypress ?
javascript 62.79% <85.29%> (+0.14%) ⬆️
python 64.58% <ø> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
...ponents/AdhocFilterEditPopoverSimpleTabContent.jsx 79.83% <83.33%> (-1.98%) ⬇️
.../src/explore/components/AdhocMetricEditPopover.jsx 56.97% <83.33%> (-16.84%) ⬇️
superset-frontend/src/common/components/Select.tsx 100.00% <100.00%> (ø)
...components/AdhocFilterEditPopoverSqlTabContent.jsx 65.51% <100.00%> (+1.23%) ⬆️
...frontend/src/views/CRUD/alert/AlertReportModal.tsx 49.46% <100.00%> (+0.26%) ⬆️
superset-frontend/src/SqlLab/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/explore/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/dashboard/App.jsx 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/setup/setupColors.js 0.00% <0.00%> (-100.00%) ⬇️
superset-frontend/src/chart/ChartContainer.jsx 0.00% <0.00%> (-100.00%) ⬇️
... and 176 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 1afe915...52690b7. Read the comment docs.

@ktmud
Copy link
Member

ktmud commented Dec 14, 2020

We only added that placeholder pretty recently. I would hate to have to see it go.

cc @eschutho

@kgabryje
Copy link
Member Author

We only added that placeholder pretty recently. I would hate to have to see it go.

cc @eschutho

Hmm I see. I'll try to look for a workaround

@junlincc
Copy link
Member

@kgabryje please progress to the next refactoring/restyling task in Explore first. we can revisit and bring the place holder back after the 18th.
I understand how valuable this new placeholder is to some users, I promise we will bring it back soon.

@rusackas
Copy link
Member

rusackas commented Dec 15, 2020

@kgabryje I think I found a way to use Emotion and the :after pseudo element to handle the placeholder feature. Check it https://codesandbox.io/s/multiple-selection-antd493-forked-pc9h7?file=/index.js

@kgabryje
Copy link
Member Author

kgabryje commented Dec 15, 2020

Thanks @rusackas, works like a charm! I needed to add some conditions when to display that labelText, because simply replacing placeholder with labelText did not work well in some cases, but other than that it seems ok! Would appreciate if you took another look
image

@kgabryje kgabryje force-pushed the refactor/explore-select branch from 0f70e40 to de2007a Compare December 15, 2020 08:10
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

@rusackas rusackas merged commit 794d318 into apache:master Dec 16, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.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 size/L 🚢 1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants