graceguo-supercat commented 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? ---------------------------------------------------------------- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
