feat: upgrade react-select and make multi-select sortable #9628
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #9628 +/- ##
==========================================
- Coverage 71.01% 66.10% -4.92%
==========================================
Files 586 592 +6
Lines 30635 30634 -1
Branches 3159 3238 +79
==========================================
- Hits 21756 20250 -1506
- Misses 8768 10197 +1429
- Partials 111 187 +76
Continue to review full report at Codecov.
|
|
@ktmud thanks for taking this on. It's no easy task. Let me know if I can help! |
ktmud
force-pushed
the
sortable-metric-input
branch
from
74a1180 to
aa9e0d8
Compare
ktmud
changed the title
ktmud
force-pushed
the
sortable-metric-input
branch
10 times, most recently
from
69c63e7 to
c419639
Compare
ktmud
changed the title
ktmud
changed the title
ktmud
changed the title
ktmud
force-pushed
the
sortable-metric-input
branch
2 times, most recently
from
a19a6ec to
3106d17
Compare
ktmud
commented
May 7, 2020
superset-frontend/cypress-base/cypress/integration/explore/control.test.js
Show resolved
Hide resolved
kristw
reviewed
May 7, 2020
superset-frontend/cypress-base/cypress/integration/explore/visualizations/histogram.js
Show resolved
Hide resolved
ktmud
force-pushed
the
sortable-metric-input
branch
5 times, most recently
from
0b0a699 to
7d0cd07
Compare
ktmud
commented
May 8, 2020
There was a problem hiding this comment.
@ All, this is finally ready for review! Apologies in advance for the giant diff.
Added some comments as pointers. For starters, the most important file to look at is https://github.com/apache/incubator-superset/pull/9628/files#diff-650ab8c9d74299035d13f427d0eaf5df
superset-frontend/cypress-base/cypress/integration/explore/control.test.js
Show resolved
Hide resolved
superset-frontend/cypress-base/cypress/integration/explore/chart.test.js
Show resolved
Hide resolved
...frontend/spec/javascripts/explore/components/AdhocFilterEditPopoverSimpleTabContent_spec.jsx
Show resolved
Hide resolved
superset-frontend/spec/javascripts/explore/components/MetricsControl_spec.jsx
Show resolved
Hide resolved
ktmud
force-pushed
the
sortable-metric-input
branch
3 times, most recently
from
8948497 to
d803d81
Compare
Upgrade `react-select`, replace `react-virtualized-select` with a custom solution implemented with `react-window`. Future plans include deprecate `react-virtualized` used in other places, too. Migrate all react-select related components to `src/Components/Select`.
TODO: add typing support for AsyncSelect props.
ktmud
force-pushed
the
sortable-metric-input
branch
from
d803d81 to
3bfabac
Compare
Codecov Report
@@ Coverage Diff @@
## master #9628 +/- ##
==========================================
+ Coverage 71.02% 71.13% +0.10%
==========================================
Files 583 589 +6
Lines 30615 30797 +182
Branches 3162 3241 +79
==========================================
+ Hits 21745 21907 +162
- Misses 8759 8779 +20
Partials 111 111
Continue to review full report at Codecov.
|
|
LGTM. |
ktmud
added a commit
to ktmud/superset-ui
that referenced
this pull request
May 21, 2020
This property for AdhocMetric has been removed by apache/superset#9628
This was referenced May 21, 2020
6 tasks
ktmud
added a commit
to ktmud/superset-ui
that referenced
this pull request
Jun 17, 2020
6 tasks
auxten
pushed a commit
to auxten/incubator-superset
that referenced
this pull request
Nov 20, 2020
* feat: upgrade react-select v1.3.0 to v3.1.0 Upgrade `react-select`, replace `react-virtualized-select` with a custom solution implemented with `react-window`. Future plans include deprecate `react-virtualized` used in other places, too. Migrate all react-select related components to `src/Components/Select`. * Fix new list view * Fix tests * Address PR comments * Fix a flacky Cypress test * Adjust styles for Select in CRUD ListView * Fix loadOptions for owners select in chart PropertiesModal TODO: add typing support for AsyncSelect props. * Address PR comments; allow isMulti in SelectControl, too * Clean up NaN in table filter values * Fix flacky test
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CATEGORY
SUMMARY
In order to enable easy reordering of selected metrics/columns, we need to upgrade
react-selectv1 and v3.This PR has done a lot of cleanups and refactoring to make this happen:
react-selectfrom1.3.0to3.1.0.react-virtualized-selectwith a custom implementation based onreact-window. Inspired by but simpler than react-windowed-select: we did not handle grouped options, but added typing.src/components/SelectandSupersetStyledSelectas the all-in-one entrypoint to all Superset styled selects. It also handles API backward compatibility for the purpose of: 1). increase usability; 2) keep changes in dependent modules minimal.WindowedSelect) will be used only when options list is long (currently > 100).src/components/StyledSelectcurrently used in the new ListView will be deprecated.Since this is a fairly large change, it'd be highly appreciated if anyone who has worked on
react-selectorSelectControlcomponent in Superset before could take a close look to these changes.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
I know I have made a promise to keep all original styles, but the UI did change a bit in the end, especially the multi-value labels. The new default UI from
react-selectdo make some sense and I don't want to add more complexity to the code just for overriding them.Before:
After:
Another difference is that in FilterBox, selected filter values will not have bars indicate frequency. The original design doesn't make much sense anyway since each selected value is not of equal length and it kind of interferes with legibility.
Before:
After:
TEST PLAN
CI & Manual tests.
ADDITIONAL INFORMATION
REVIEWERS
@kristw @graceguo-supercat @evans @villebro @suddjian @mistercrunch @etr2460 @nytai