[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #11814: feat(dashboard): Dashboard-Native Filters

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

graceguo-supercat edited a comment on pull request #11814:
URL: https://github.com/apache/incubator-superset/pull/11814#issuecomment-740472118


   @suddjian Thanks for making this PR. this is big feature! 
   Here are some thoughts after a quick look:
   - After this PR merged, what kind of users you will suggest them to enable this feature, while what users should not?
   
   - From description, it seems there are quite some regression from old dashboard filters behavior. I assume this PR will reach certain milestones for the filter components. Could you list what user flows are ready to use after this PR? 
   
   - You changed 108 files so far, but > 50% of changes are from unit test for many different components: datasource editor, sql lab, explore view, save modal...why these changes have to be in this PR? Could you make another PR for those unit test updates?
   
   - A few new components have unit tests:
   superset-frontend/spec/javascripts/dashboard/components/nativeFilters/FilterBar_spec.tsx
   superset-frontend/spec/javascripts/dashboard/components/nativeFilters/FilterConfigurationLink_spec.tsx
   superset-frontend/spec/javascripts/dashboard/components/nativeFilters/NativeFiltersModal_spec.tsx
   superset-frontend/spec/javascripts/dashboard/components/nativeFilters/ScopingTree_spec.tsx
   you prefer to use enzyme not new recommended `react test library`? In the enzyme test, are you sure you always want to use `mount` instead of `shallow`? [ this link explains the difference](https://stackoverflow.com/questions/38710309/when-should-you-use-render-and-shallow-in-enzyme-react-tests) 
   
   - What is your plan to migrate old filter_box to native filter component? are they going to co-exist in the same dashboard?
   
   - Could you introduce what is new viz_type `filter_select`, and what is different from `filter_box`?


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



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