You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/05/20 13:18:10 UTC

[GitHub] [superset] michael-s-molina opened a new pull request, #20143: feat: Adds the "Select all" option to the Select component

michael-s-molina opened a new pull request, #20143:
URL: https://github.com/apache/superset/pull/20143

### 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](https://ux.stackexchange.com/questions/43937/how-should-the-select-all-select-when-paginating-elements). 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
https://user-images.githubusercontent.com/70410625/169524505-ac812e6c-29ab-4671-a198-03600a02b94d.mov

### TESTING INSTRUCTIONS
TODO

### ADDITIONAL INFORMATION
- [x] Has associated issue:
- https://github.com/apache/superset/issues/19426
- https://github.com/apache/superset/pull/19979
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [x] Introduces new feature or API
- [ ] Removes existing feature or API

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

michael-s-molina commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1132898319

   @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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] cccs-RyanK commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

cccs-RyanK commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1153869897

   @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!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

michael-s-molina commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1201450874

   > 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](https://github.com/apache/superset/pull/20466#issuecomment-1163436259). We have completed steps 1-3 with https://github.com/apache/superset/pull/20466 and https://github.com/apache/superset/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina closed pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

michael-s-molina closed pull request #20143: feat: Adds the "Select all" option to the Select component
URL: https://github.com/apache/superset/pull/20143


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] EugeneTorap commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

EugeneTorap commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1201411129

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] michael-s-molina commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

michael-s-molina commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1248441353

> 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 https://github.com/apache/superset/pull/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.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [superset] lmaloney commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

lmaloney commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1167602357

   Thank you for taking a look at this!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] EugeneTorap commented on pull request #20143: feat: Adds the "Select all" option to the Select component

Posted by GitBox <gi...@apache.org>.

EugeneTorap commented on PR #20143:
URL: https://github.com/apache/superset/pull/20143#issuecomment-1248356955

   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 by dataset name in Dataset page and we are redirected to explore page and see `all columns` result after report execution. Because the vast majority of users use table viz with all columns.
   What do you think about it?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org