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

chore: Split Select component into Async and Sync components #20466

Conversation

cccs-RyanK
Copy link
Contributor

SUMMARY

Splits the Synchronous and Asynchronous behaviour of the Select component into two separate components. It's very clear that the behaviors are different and we can significantly reduce code complexity if we treat them as different components.

This task was outlined as a part of this PR by @michael-s-molina : #20143

Most of the use cases for the select component are synchronous at the moment, so the default "Select" component will remain the synchronous Select. "AsyncSelect" is the new component for asynchronous use cases. For this PR, the files remain identical to ensure nothing will be broken, but references to the Select Component that have asynchronous options have been changed to reference AsyncSelect.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

Changed files to reference AsyncSelect if needed
…LDN-1484-Split-Select-Component-into-Async-and-Sync-Components
@michael-s-molina
Copy link
Member

Thanks for the PR @cccs-RyanK!

Some suggestions to complete this PR and the first phase:

1 - You can include this line in src/components/index.ts to be able to import the AsyncSelect directly from src/components:

export { default as AsyncSelect } from './Select/AsyncSelect';

And then import the component with:

import { AsyncSelect } from 'src/components';

2 - Don't forget to remove the imports of the sync Select when changing to AsyncSelect.

3 - Remove the async - prefix of the tests in AsyncSelect.test.tsx

4 - Remove the static - prefix of the tests in Select.test.tsx

5 - Remove the tests in Select.test.tsx that contain the async - prefix

6 - Split the Select Storybook (Select.stories.tsx and AsyncSelect.stories.tsx)

Thanks again! This will really reduce code complexity!

@michael-s-molina
Copy link
Member

michael-s-molina commented Jun 22, 2022

For anyone interested, here are the phases for improving the component:
1 - Split the component into sync and async versions while preserving the code (this PR)
2 - Remove the unnecessary code from the sync version (next PR)
3 - Remove the unnecessary code from the async version
4 - Implement the "Select all" feature for the sync version (90% of the codebase)
5 - Implement the "Select all" feature for the async version

My suggestion is that we implement each phase in a specific PR.

@rusackas @cccs-RyanK

@cccs-RyanK cccs-RyanK marked this pull request as ready for review June 23, 2022 12:48
@michael-s-molina michael-s-molina changed the title Split Select component into Async and Sync components chore: Split Select component into Async and Sync components Jun 24, 2022
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.

Second review completed 👍🏼

We also need to remove any references to loadOptions (async) in the Select.test.tsx 😉

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.

Thanks for the changes @cccs-RyanK! I think it's just a matter of fixing CI errors now.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #20466 (c5d9829) into master (e7b965a) will decrease coverage by 0.01%.
The diff coverage is 82.84%.

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

@@            Coverage Diff             @@
##           master   #20466      +/-   ##
==========================================
- Coverage   66.85%   66.84%   -0.02%     
==========================================
  Files        1754     1755       +1     
  Lines       65757    65996     +239     
  Branches     6952     7030      +78     
==========================================
+ Hits        43964    44115     +151     
- Misses      20029    20092      +63     
- Partials     1764     1789      +25     
Flag Coverage Δ
javascript 51.94% <82.84%> (+0.07%) ⬆️

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

Impacted Files Coverage Δ
...perset-frontend/src/addSlice/AddSliceContainer.tsx 66.66% <ø> (ø)
...frontend/src/components/DatabaseSelector/index.tsx 88.88% <ø> (ø)
...end/src/components/Datasource/DatasourceEditor.jsx 65.61% <ø> (ø)
...src/dashboard/components/PropertiesModal/index.tsx 62.98% <ø> (ø)
...ersConfigModal/FiltersConfigForm/DatasetSelect.tsx 40.00% <ø> (ø)
...d/src/explore/components/PropertiesModal/index.tsx 63.93% <ø> (ø)
...frontend/src/views/CRUD/alert/AlertReportModal.tsx 52.08% <ø> (ø)
...set-frontend/src/components/Select/AsyncSelect.tsx 82.84% <82.84%> (ø)
superset-frontend/src/components/Select/Select.tsx 67.78% <0.00%> (-19.67%) ⬇️

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 e7b965a...e43cdd3. Read the comment docs.

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.

LGTM. Thank you for all the work @cccs-RyanK! I can't wait for the next phases!

@michael-s-molina michael-s-molina merged commit 1109fe5 into apache:master Jul 7, 2022
jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Jul 13, 2022
…20466)

* Created AsyncSelect Component
Changed files to reference AsyncSelect if needed

* modified import of AsyncSelect, removed async tests and prefixes from select tests

* fixed various import and lint warnings

* fixing lint errors

* fixed frontend test errors

* fixed alertreportmodel tests

* removed accidental import

* fixed lint errors

* updated async select

(cherry picked from commit 1109fe5)
akshatsri pushed a commit to charan1314/superset that referenced this pull request Jul 19, 2022
…20466)

* Created AsyncSelect Component
Changed files to reference AsyncSelect if needed

* modified import of AsyncSelect, removed async tests and prefixes from select tests

* fixed various import and lint warnings

* fixing lint errors

* fixed frontend test errors

* fixed alertreportmodel tests

* removed accidental import

* fixed lint errors

* updated async select
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 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 preset:2022.27 size/XXL 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants