fix: Select refactoring known issues #16666

geido · 2021-09-10T15:11:10Z

SUMMARY

The goal of this PR is to solve the known issues of the new Antd-based Select component, as follows:

When showSearch is ON and labels are customized with JSX, the Select can only search by value. For instance, Explore - Customise - COLOR SCHEME - Search for airbnb - won’t work - Will only work with searching bnb as the value used is bnbColors
When using labels with custom JSX the rendered HTML is showing label=[Object, Object]
Write a string, then delete it entirely. The string is still there and selected
With allowNewOptions true, type a non-existent option, then delete it char by char. Not the entire string is deleted.
When trying to create a new option, but the search is returning matching options, you won’t be able to create the new option, as the Select will instead pick the first matching option

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

superset-frontend/src/components/Select/Select.tsx

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

codecov · 2021-09-13T19:27:19Z

Codecov Report

Merging #16666 (f01d3cf) into master (5ac8ac0) will increase coverage by 0.01%.
The diff coverage is 91.25%.

❗ Current head f01d3cf differs from pull request most recent head f9c0367. Consider uploading reports for the commit f9c0367 to get more accurate results

@@            Coverage Diff             @@
##           master   #16666      +/-   ##
==========================================
+ Coverage   76.93%   76.95%   +0.01%     
==========================================
  Files        1007     1007              
  Lines       54112    54129      +17     
  Branches     7346     7366      +20     
==========================================
+ Hits        41633    41653      +20     
+ Misses      12239    12236       -3     
  Partials      240      240

Flag	Coverage Δ
javascript	`71.28% <91.25%> (+0.03%)`	⬆️

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

Impacted Files	Coverage Δ
...ntend/src/explore/components/ExploreChartPanel.jsx	`14.28% <0.00%> (-0.72%)`	⬇️
...perset-frontend/src/addSlice/AddSliceContainer.tsx	`60.00% <50.00%> (+3.07%)`	⬆️
superset-frontend/src/components/Select/Select.tsx	`91.66% <97.05%> (+1.70%)`	⬆️
superset-frontend/src/components/Select/utils.ts	`96.15% <100.00%> (ø)`
...onfigModal/FiltersConfigForm/FiltersConfigForm.tsx	`73.27% <100.00%> (+0.07%)`	⬆️
...nd/src/explore/components/DataTablesPane/index.tsx	`75.49% <100.00%> (+0.24%)`	⬆️
...explore/components/controls/ColorSchemeControl.jsx	`89.28% <100.00%> (+0.39%)`	⬆️

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 5ac8ac0...f9c0367. Read the comment docs.

michael-s-molina · 2021-09-13T19:37:38Z

superset-frontend/src/components/Select/Select.tsx

+    setSelectValue(value);
+  }, [value]);
+
+  useEffect(() => {


Should we only add a new option if allowNewOptions is true? Currently is adding even if allowNewOptions is false. If we do this, what will happen when allowNewOptions is false and an unknown value is passed? Should we discard it and override the selectedValue?

I believe it makes sense to keep it when allowNewOptions is both false and true. I don't see the danger in doing that but I see the danger in discarding a value. What do you think?

…select_refactoring_known_issues

…o/incubator-superset into fix/select_refactoring_known_issues

michael-s-molina

LGTM. Thanks for the fixes!

* Clean up and reorganize effects * Enhance optionFilterProps * Render custom label * Remove prop filtering * Create options * Create option from value in single mode * Change to customLabel * Show search by default * Update superset-frontend/src/components/Select/Select.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/src/components/Select/Select.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/src/components/Select/Select.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Apply minor changes * Fixes a bug that was failing CI * Adds more tests to the component * Apply customLabel in ColorSchemeControl * Remove customLabel from rendered Option * Hide No data when allowNewOptions * Remove unnecessary prop from tests * Adjust loading height * Show no data with fetchOnlyOnSearch Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>

geido added 2 commits September 10, 2021 17:30

Clean up and reorganize effects

15fdf9d

Enhance optionFilterProps

4c032da

superset-github-bot bot added the Flexiana label Sep 10, 2021

pull-request-size bot added the size/L label Sep 10, 2021

geido added 4 commits September 10, 2021 20:08

Render custom label

df58428

Remove prop filtering

37620ce

Create options

36935a8

Create option from value in single mode

841f987

michael-s-molina changed the title ~~Fix/select refactoring known issues~~ fix: select refactoring known issues Sep 13, 2021

geido changed the title ~~fix: select refactoring known issues~~ fix: Select refactoring known issues Sep 13, 2021

geido marked this pull request as ready for review September 13, 2021 13:45

geido requested a review from michael-s-molina September 13, 2021 13:45

geido added 2 commits September 13, 2021 17:30

Change to customLabel

cfeb776

Show search by default

e757911

michael-s-molina reviewed Sep 13, 2021

View reviewed changes

geido and others added 4 commits September 13, 2021 19:11

Update superset-frontend/src/components/Select/Select.tsx

c1dc254

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/src/components/Select/Select.tsx

669da5e

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Update superset-frontend/src/components/Select/Select.tsx

f15396c

Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>

Apply minor changes

f8b3d99

geido requested a review from michael-s-molina September 13, 2021 16:21

Fixes a bug that was failing CI

b0631ce

Adds more tests to the component

7e161ee

michael-s-molina reviewed Sep 13, 2021

View reviewed changes

geido added 6 commits September 14, 2021 14:29

Merge branch 'master' of https://github.com/apache/superset into fix/…

39440f8

…select_refactoring_known_issues

Merge branch 'fix/select_refactoring_known_issues' of github.com:geid…

974586f

…o/incubator-superset into fix/select_refactoring_known_issues

Apply customLabel in ColorSchemeControl

b0a0ff2

Remove customLabel from rendered Option

6c28548

Hide No data when allowNewOptions

98de34d

Remove unnecessary prop from tests

ee78d43

geido added 2 commits September 14, 2021 17:07

Adjust loading height

bf5987a

Show no data with fetchOnlyOnSearch

f9c0367

michael-s-molina approved these changes Sep 14, 2021

View reviewed changes

geido merged commit fecd412 into apache:master Sep 14, 2021

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.4.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Select refactoring known issues #16666

fix: Select refactoring known issues #16666

geido commented Sep 10, 2021 •

edited

codecov bot commented Sep 13, 2021 •

edited

michael-s-molina Sep 13, 2021

geido Sep 13, 2021

michael-s-molina Sep 13, 2021

michael-s-molina left a comment

fix: Select refactoring known issues #16666

fix: Select refactoring known issues #16666

Conversation

geido commented Sep 10, 2021 • edited

SUMMARY

ADDITIONAL INFORMATION

codecov bot commented Sep 13, 2021 • edited

Codecov Report

michael-s-molina Sep 13, 2021

Choose a reason for hiding this comment

geido Sep 13, 2021

Choose a reason for hiding this comment

michael-s-molina Sep 13, 2021

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

geido commented Sep 10, 2021 •

edited

codecov bot commented Sep 13, 2021 •

edited