Fixing explore actions & slice controller interactions #1292
Conversation
mistercrunch
force-pushed
the
bkp_fix_exploreactions
branch
from
bfbc4e1 to
9ecf187
Compare
mistercrunch
force-pushed
the
bkp_fix_exploreactions
branch
from
9ecf187 to
9244c26
Compare
| $(document).ready(function () { | ||
| const data = $('.slice').data('slice'); | ||
|
|
||
| initExploreView(); | ||
|
|
||
| slice = px.Slice(data); | ||
| slice = px.Slice(data, exploreController); | ||
|
|
||
| $('.slice').data('slice', slice); |
There was a problem hiding this comment.
Why do we write the slice back to the DOM? Later, slice.data or slice.viewSqlLQuery will change and the representation in the DOM is outdated. Moreover, it is used nowhere.
There was a problem hiding this comment.
good catch, I'm removing it and making sure things still work. We're re-writing the explore view to react so I don't want to go too far in the refactoring, but that's an easy win.
| flts = dashboard.effectiveExtraFilters(sliceId); | ||
| flts = encodeURIComponent(JSON.stringify(flts)); | ||
| } | ||
| if (controller.type === 'dashboard') { |
There was a problem hiding this comment.
- origJsonEndpoint with the params for the chart is only used in the dashboard to be able to remove the extra_filters. Hence, I would save it in the dashboard.
- The form input with the params for the chart is only used in explore.
- In the default case, e.g. standalone, the query params are from the slide itself.
Thus, I would put the query string generation in the controller interface in use the default case when it is not implemented.
There was a problem hiding this comment.
Would you please describe the interface with a few comments. e.g. describe the case when each function is called.
There was a problem hiding this comment.
thanks for the review, I know there's much more refactoring to do here to move all the logic in the right places, I want to merge this quickly-ish to fix the bug and we can see how much more refactoring we want to do here in the future but most likely not much knowing that we're re-writing the whole explore view...
|
thanks for this fix! 🎉 |
mistercrunch
added
🏷️ bot
The idea here is
controllerInterface, a common interface for slices to interact with their context, or controller (explore, dashboard or standalone implement this interface).For instance, when a slice is done loading, it calls it's controller
donemethod, which will trigger different things depending on the context.This allows for moving logic specific to dashboards & explore back into those objects, away from the Slice object itself.