[GitHub] [incubator-superset] suddjian commented on pull request #11814: feat(dashboard): Dashboard-Native Filters

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

suddjian commented on pull request #11814:
URL: https://github.com/apache/incubator-superset/pull/11814#issuecomment-741024435


   > @suddjian Thanks for making this PR. this is big feature!
   
   Thanks, this is a big collaboration with many teammates trying out a new cross-org development process, so many thanks to @pkdotson, @villebro, @rusackas, @simchaNielsen, @amitmiran137, and @agatapst for all the contributions!
   
   > After this PR merged, what kind of users you will suggest them to enable this feature, while what users should not?
   
   Deferring to @junlincc on this one.
   
   > * 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?
   
   - Creating a filter from a dashboard as a dashboard owner (including scoping the filter)
   - Setting filter values as a dashboard user
   
   That's really it. Goal with this PR is to get the core basic functionality merged so that we can make future contributions incrementally. That way it should be easier for everyone to review and critique individual sub-features.
   
   > * 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?
   
   That's a really good point. The changes to tests were necessary because we upgraded Redux to the latest version, which broke many tests that implicitly depended on implementation details of `connect()`. I will look into opening that as a separate PR.
   
   > * 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)
   
   I think those tests were written before the SIP was passed. My understanding is that everyone on our team pretty much prefers RTL, but we'd rather not change code that was already written. I defer to @pkdotson on whether to use `shallow` or `deep`. I have often run into technical limitations with shallow rendering on function components, so I suspect that could be a reason.
   
   > * What is your plan to migrate old filter_box to native filter component? are they going to co-exist in the same dashboard?
   
   For now, they will co-exist. The `filter_box` viz and native filters do not interfere with each other, although you could in theory include an old `filter_box` in the scope of a native filter. We don't really expect users to use both, but they could if they wanted to for some reason.
   
   I don't think it would make sense to migrate the `filter_box` until after native filters are more mature.
   
   > * Could you introduce what is new viz_type `filter_select`, and what is different from `filter_box`?
   
   This is the best implementation we were able to get to use the query api without duplicating too much logic. It should really be thought of as a Plugin rather than a viz type, but the Plugin api isn't quite robust enough yet to fully separate those concepts. @villebro may be able to say more.


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