[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #8587: [dashboard filter scope] force show filter_box as unchecked

[GitBox <gi...@apache.org>]

graceguo-supercat opened a new pull request #8587: [dashboard filter scope] force show filter_box as unchecked
URL: https://github.com/apache/incubator-superset/pull/8587
 
 
   ### CATEGORY
   
   Choose one
   
   - [ ] Bug Fix
   - [ ] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   We do not apply filter on filter_box itself, so in the scope selector modal, the filter_box's checkbox is disabled. But when user clicks on tab or other parent level node's checkbox, react-checkbox-tree framework still set the value for disabled nodes. So sometimes this will cause confusing. For example: in the UI no chart is selected but the root show half-checked:
   ![image](https://user-images.githubusercontent.com/27990562/68974317-4f521880-07a5-11ea-8c14-3ee2f96fc653.png)
   
   This PR will add an extra logic that enforce filter_box's state is unchecked, but still show as disabled. This is still not **_perfect_**, because when user checked parent's checkbox, they are expected to see all children nodes are also checked. In this case, filter_box's state is still unchecked. There is a proposal that to compare all the nodes under the parent, and ignore the filter_box's state. But the nodes are in the tree structure, given a filter_box chartId, find its parent, and find other siblings. this computation is not cheap, and will be triggered very frequent.
   
   Given the performance impact is big and corner cases is small, I propose this cheaper solution.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   @etr2460 @kristw 
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

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