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: Adds the "Select all" option to the Select component #20143

Closed
wants to merge 1 commit into from

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented May 20, 2022

SUMMARY

This PR aims to add the "Select all" feature to the Select component. Adding this feature to a synchronous Select is really simple. The problem arises when adding "Select all" to an asynchronous, paginated, searchable, and keep selected on top Select. We have many technical challenges and design considerations as you can see here. Some of the requirements we need to address:

  • Select all items when only some pages are loaded on the client-side. Selecting only the visible items was discarded because, in our context, the majority of times the user is actually selecting all values. We also don't want to force the user to load all items before being able to select them.
  • Load new items as selected when "Selected all" is active
  • Unselect a set of items when "Select all" is active and not all pages have been loaded on the client-side
  • Keep the order of selected items during user interactions
  • Automatically "Select all" when all items have been selected and not all pages have been loaded on the client-side. One example here is when the user opens the Select, one page is loaded, the user clicks on "Select all", unselects one item, and selects the same item again. At this point, we should go back to the "Select all" mode even though we only have one page loaded.
  • Handle the payload size. We shouldn't send all selected items to the server-side because we don't know how many can exist, which could result in huge payload size. What we need to do is to come up with a payload that represents the user selection in an efficient way. One example would be if a user selects all items and unselects a subset, we could send a value representing the "Select all" alongside the subset of unselected items.
  • Write tests that are able to scroll the component, trigger new page loads, and test the above scenarios.
  • Expand the storybook to account for the new requirements.
  • We also need to split the component into two (sync and async). It's very clear that the behaviors are different and we can significantly reduce code complexity if we treat them as different components.
  • Add support to the new payload format to the API endpoints

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-05-20.at.8.57.20.AM.mov

TESTING INSTRUCTIONS

TODO

ADDITIONAL INFORMATION

@michael-s-molina
Copy link
Member Author

@ktmud @cccs-RyanK @lmaloney Currently, the work consisted of creating a POC to address some of the above requirements but we still have a lot of work ahead. Unfortunately, this is being deprioritized in our roadmap so I won't have much time available to continue the work right now. I'll try to continue whenever I have some time available. I'm sharing the draft PR in case anyone wants to tackle some of the efforts or contribute to the discussion. To check the POC, you just need to go to the Select Storybook.

@cccs-RyanK
Copy link
Contributor

@michael-s-molina I have some time for this now so I plan on adding to the work you did to finish this feature. Please feel free to reach out if there is any problem with that!

@lmaloney
Copy link

Thank you for taking a look at this!

@EugeneTorap
Copy link
Contributor

Hi @michael-s-molina @cccs-RyanK. This PR is extremely important.
Any timeline on when this might be merged?

@michael-s-molina
Copy link
Member Author

Hi @michael-s-molina @cccs-RyanK. This PR is extremely important. Any timeline on when this might be merged?

Hi @EugeneTorap. We're following the plan outlined here. We have completed steps 1-3 with #20466 and #20690. I think we can deliver step 4 this month, which will cover 90% of the cases. @cccs-RyanK let us know if you think otherwise.

@EugeneTorap
Copy link
Contributor

EugeneTorap commented Sep 15, 2022

Hi @michael-s-molina. Can we rebase it from master branch?
There's a good idea, use QUERY MODE is Raw Records in table viz and select all columns by default. For example when we click a dataset name in Dataset page then we are redirected to explore page and see all columns result after chart execution. Because the vast majority of users use table viz with all columns.
What do you think about it?

@michael-s-molina
Copy link
Member Author

Hi @michael-s-molina. Can we rebase it from master branch? There's a good idea, use QUERY MODE is Raw Records in table viz and select all columns by default. For example when we click a dataset name in Dataset page then we are redirected to explore page and see all columns result after chart execution. Because the vast majority of users use table viz with all columns. What do you think about it?

Hi @EugeneTorap. This draft is only meant to be a POC. We just merged #21094 and now the next step is to add Select All to the sync version. Async support will come next, in a different PR than this one. I'll close this PR to avoid further confusion.

The Select All in Explore is a specific case and there's a discussion about it being conducted by @kasiazjc @cccs-RyanK. Here we're focusing more on the generic feature for all selects. @kasiazjc @cccs-RyanK I would appreciate it if you could post the discussion thread or any doc reference so that @EugeneTorap can participate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants