-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DEV-8698] Applies Filters on MultiDash Switch #1389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpallansch this lgtm and can be squash/merged
@@ -265,6 +265,10 @@ export default function CdcDashboard({ initialState, isEditor = false, isDebug = | |||
loadAPIFilters(config.dashboard.sharedFilters) | |||
}, [isEditor, isPreview]) | |||
|
|||
useEffect(() => { | |||
updateDataFilters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you move this into the first useEffect right after loadAPIFilters it would also work. Did you try that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is related to this issue you mentioned on 3. in dev-8409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is related to this issue you mentioned on 3. in dev-8409
I don't believe so, 3 in 8409 is related to the value not being checked for null in packages/dashboard/src/helpers/generateValuesForFilter.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you move this into the first useEffect right after loadAPIFilters it would also work. Did you try that?
Yes, I had tried that, but the useEffect only has isEditor and isPreview as dependencies, so it doesn't fire when the dashboard multi-tab is changed. I can combine the useEffects possibly, including state.config?.activeDashboard on the first useEffect, but then API filters will reload every time the multi-tab changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshlacey I combined the useeffects in the latest commit, let me know if looks better now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should update the shared filters and then pass the result to loadAPIFilters. that's what I'm doing in the dashboard filters refactor branch
const clonedState = _.cloneDeep(state) | ||
if (sharedFilters) clonedState.config.dashboard.sharedFilters = sharedFilters | ||
const newFilteredData = getFilteredData(clonedState) | ||
dispatch({ type: 'SET_FILTERED_DATA', payload: newFilteredData }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does the filtered data have to do with the values on the dashboard filter?
No description provided.