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

feat: Select component (Iteration 1) #15121

Merged
merged 19 commits into from
Jun 16, 2021
Merged

feat: Select component (Iteration 1) #15121

merged 19 commits into from
Jun 16, 2021

Conversation

geido
Copy link
Member

@geido geido commented Jun 11, 2021

SUMMARY

This PR introduces a new Select component based on the Antdesign Select. The goal of the refactoring is to eliminate all the various instances of selects throughout the codebase and merge them into one, while getting rid of the react-select entirely.

PROPS

At the moment only some of the Antd props have been exposed in the interface on top of the custom props. This is to make sure that the Select component is used with the intended behaviors. However, as the implementation of the new component goes, we might realize that other default props should be exposed.

Prop Name Origin Type Description Prop exposed Default behavior (prop not exposed)
mode Antd multiple OR tags Set mode of Select x
truncateOptions Custom true (default) OR false When true it truncates the options with a long text and shows a tooltip. When false, it wraps options with a long text to a new line. x
maxTagCount Antd number OR responsive Max tag count to show. responsive will cost render performance x
showSearch Antd true OR false (default) Whether show search input in single mode x
allowNewOptions Custom true OR false It allows to create a new option when no options match the search. This is true by default when the property mode is set to tags OR multiple. Otherwise it's false by default. x
options Antd { label, value }[] OR Promise => { label, value }[] Select options. It can accept an array or a promise that resolves an array or rejects x
loading Antd true OR false (default) Indicate loading state x
onChange Antd function(value, option:Option OR Array) Called when select an option or input value change x
placeholder Antd ReactNode Placeholder of select x
value Antd string OR string[]
number OR number[]
LabeledValue OR LabeledValue[]
Current selected option x
tokenSeparators Antd string[] Separator used to tokenize on tag and multiple mode x
autoFocus Antd true OR false (default) Get focus by default x
allowClear Antd true OR false (default) Show clear button x
disabled Antd true OR false (default) Whether disabled select x
filterOption Antd true (default) OR false OR function(inputValue, option) If true, filter options by input, if function, filter options against it. The function will receive two arguments, inputValue and option, if the function returns true, the option will be included in the filtered set; Otherwise, it will be excluded x
aria-label Antd string Aria label (for assistive tech)

x
getPopupContainer Antd function(triggerNode)

Default:
() => document.body
Parent Node which the selector should be rendered to. Default to body. When position issues happen, try to modify it into scrollable content and position it relative. x
name Custom string Used for the test cases x
notFoundContent Antd ReactNode Specify content to show when no result matches x
header Custom ReactNode Header of the Select x
paginatedFetch Custom true OR false (default) Makes the async select paginated x

CUSTOM FEATURES

Few abilities have been added on top of the default Antd Select component, as follows:

  • The creation of custom options for single mode
  • Options as an array or as a Promise to fetch data from the server (more information below in Async Select section)
  • Header as a prop
  • Sorting of the selected options at the top of the dropdown
  • Automatic scroll at the top of the dropdown

ASYNC SELECT

The Select component now has the ability to fetch data. The options property can accept a Promise. The Select component will do this for you:

  • Runs the fetch when the component mounts to load the initial options
  • It will check if the searched value already exists in the options and won't run the fetch in that case
  • It allows pagination when the property paginatedFetch is set and will automatically run the fetch by passing the current page as the user scrolls through the options
  • It will handle and print errors

WHAT'S MISSING (Upcoming in Iteration 2)

  • Paginate few px before the bottom of the scroll for better UX
  • Check/Discuss whether we need the onPaste and on closeOnScrollOut props
  • Remove the usage of any from the implementation
  • When allowNewOptions is true, showSearch should be true as well in Storybook
  • Our interface should only expose single and multiple modes
  • Test cases

This first iteration should be merged as it does not affect any existing functionality. @michael-s-molina will take over the "WHAT'S MISSING" and any other change request in a separate PR as soon as this is merged (Iteration 2).

@geido geido marked this pull request as draft June 11, 2021 15:28
@michael-s-molina
Copy link
Member

@geido First thoughts:

  • You can rename AntdSelect and AntSelect.stories to Select and Select.stories using the index to resolve any name conflicts.
  • I think it would be nice to simulate a service API for the storybook. We can support search, pagination and also throw errors to test the error handling.
  • You can use React useReducer to manipulate multiple state values if the number of states increases.
  • Replace handleOnDeselect = (option: any) with handleOnDeselect = (value: string | number | AntdLabeledValue)
  • Replace handleOnSelect = (option: any) with handleOnSelect = (value: string | number | AntdLabeledValue)
  • It would be nice to add one Select on each corner of the viewport on the storybook page. Then we can test the drop-down behavior for each corner and when scrolling.

@geido
Copy link
Member Author

geido commented Jun 14, 2021

@geido First thoughts:

  • You can rename AntdSelect and AntSelect.stories to Select and Select.stories using the index to resolve any name conflicts.

This is just to simplify the review and to avoid that the new implementation would appear as changes made on the original file. Once this PR is merged, we can just rename it.

  • I think it would be nice to simulate a service API for the storybook. We can support search, pagination, and also throw errors to test the error handling.

I have enhanced the storybook to provide an Async Select dedicated story. However, I am looking to do more improvements there as soon as I am done with the rest of the implementation.

  • You can use React useReducer to manipulate multiple state values if the number of states increases.

I am definitely going to if the number of changes to the state should increase.

  • Replace handleOnDeselect = (option: any) with handleOnDeselect = (value: string | number | AntdLabeledValue)
  • Replace handleOnSelect = (option: any) with handleOnSelect = (value: string | number | AntdLabeledValue)

I tried, however, typescript still complains. I am avoiding dedicating too much time to this and leaving it for when the main implementation is completed.

  • It would be nice to add one Select on each corner of the viewport on the storybook page. Then we can test the drop-down behavior for each corner and when scrolling.

This is now done.

Thank you!

@geido geido changed the title feature: Select component feat: Select component Jun 14, 2021
allowNewOptions?: boolean;
ariaLabel: string;
header?: ReactNode;
name?: string; // discourage usage
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious why this a prop we don't want to use anymore?

Copy link
Member Author

@geido geido Jun 15, 2021

Choose a reason for hiding this comment

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

There's has been a discussion for this prop. The consensus is that aria-label is a better option

@geido geido changed the title feat: Select component feat: Select component (Iteration 1) Jun 15, 2021
@geido geido marked this pull request as ready for review June 15, 2021 19:09
@rusackas
Copy link
Member

/testenv up

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #15121 (5d050d4) into master (7dc0cee) will decrease coverage by 0.24%.
The diff coverage is 0.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15121      +/-   ##
==========================================
- Coverage   77.40%   77.16%   -0.25%     
==========================================
  Files         969      971       +2     
  Lines       50013    50170     +157     
  Branches     6431     6483      +52     
==========================================
  Hits        38712    38712              
- Misses      11098    11255     +157     
  Partials      203      203              
Flag Coverage Δ
javascript 71.74% <0.63%> (-0.48%) ⬇️

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

Impacted Files Coverage Δ
...rset-frontend/src/components/Select/AntdSelect.tsx 0.00% <0.00%> (ø)
...rontend/src/components/Select/DeprecatedSelect.tsx 86.73% <ø> (ø)
superset-frontend/src/components/Select/styles.tsx 84.72% <ø> (ø)
superset-frontend/src/components/Select/utils.ts 66.66% <0.00%> (-27.46%) ⬇️
superset-frontend/src/components/index.ts 0.00% <0.00%> (ø)
superset-frontend/src/components/Select/index.ts 100.00% <100.00%> (ø)

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 7dc0cee...5d050d4. Read the comment docs.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://54.185.132.105:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

I don't see anything majorly wrong. Looks good to go for the next iteration.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

I think this first iteration is great. Thanks for organizing the work so well.

We don't have any conflicts with the other Select component and the new Select is only being used in the Storybook so we can merge it and continue to iteration 2.

@rusackas rusackas merged commit d578ae9 into apache:master Jun 16, 2021
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@rusackas
Copy link
Member

rusackas commented Jun 19, 2021

I think this first iteration is great. Thanks for organizing the work so well.

+💯 This is some awesome foundational work, which is going to put Superset on a much more clean and consistent path with these components. Thank you also for the great pre-coding audit process of all the features and props that our "select party" offered.

For those who haven't seen it, this new component aims to (eventually) replace all of these:
image
(hat tip to @michael-s-molina for gathering all these specimens)

cccs-RyanS pushed a commit to CybercentreCanada/superset that referenced this pull request Dec 17, 2021
* Implement initial structure

* Add aria-label

* Rename files

* Refactor single mode new options

* Clean up

* Add select at every corner in storybook

* Clean up

* Add pagination

* Move selected options at the top

* Clean up

* Add license

* Refactor

* Improve pagination

* Fetch when allowNewOptions

* Clean up
QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 29, 2021
* Implement initial structure

* Add aria-label

* Rename files

* Refactor single mode new options

* Clean up

* Add select at every corner in storybook

* Clean up

* Add pagination

* Move selected options at the top

* Clean up

* Add license

* Refactor

* Improve pagination

* Fetch when allowNewOptions

* Clean up
cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
* Implement initial structure

* Add aria-label

* Rename files

* Refactor single mode new options

* Clean up

* Add select at every corner in storybook

* Clean up

* Add pagination

* Move selected options at the top

* Clean up

* Add license

* Refactor

* Improve pagination

* Fetch when allowNewOptions

* Clean up
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.3.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/XXL 🚢 1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants