-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(insights): Add edit button for dashboard filters #21841
Conversation
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
Size Change: +524 B (0%) Total Size: 1.05 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
@mariusandra and @Twixes is this something that would make sense? |
In general, improving this makes sense, but I'd change the flow here:
Is it easy to add a "soft apply" on filter change? Also, do we want to show if any filters are applied? A visually easy way to do it would be a red notification bubble/dot with the number of filters inside. |
Thanks for the input!
Yeah this soft apply or preview is exactly the problem.. we don't have it. It saves the whole dashboard and only this applies the filters to all insights and then you can recalculate them to see them. Not sure what to make of it. Maybe we make all insights grey when you adjust the filters until you save or cancel? 🙈
Good idea. And that also leads me to the realization that now with my change you don't see what this dashboard's filter and time is. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
31526a6
to
16e9855
Compare
40be2aa
to
a743bd5
Compare
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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 new option is good enough to try UX-wise. Code wise, I think the "stale" insight card state is no longer needed (adding dead code 🤔), and I also think a bit of state management could move from React to the Kea layer... possibly?
const [editMode, setEditMode] = useState(false) | ||
const [tempDates, setTempDates] = useState<Dates>({ | ||
dateFrom: dashboardFilters?.date_from, | ||
dateTo: dashboardFilters?.date_to, | ||
}) | ||
const [tempProperties, setTempProperties] = useState<AnyPropertyFilter[] | undefined>( | ||
dashboard?.filters.properties ?? undefined | ||
) | ||
|
||
useEffect(() => { | ||
const hasPendingChanges = | ||
tempDates.dateFrom !== dashboardFilters?.date_from || | ||
tempDates.dateTo !== dashboardFilters?.date_to || | ||
JSON.stringify(tempProperties) !== JSON.stringify(dashboard?.filters.properties) | ||
if (onPendingChanges) { | ||
onPendingChanges(hasPendingChanges) | ||
} | ||
}, [tempDates, tempProperties]) | ||
|
||
const handleSave: () => void = () => { | ||
if (!canEditDashboard) { | ||
return | ||
} | ||
setDates(tempDates.dateFrom ?? null, tempDates.dateTo ?? null) | ||
if (tempProperties) { | ||
setProperties(tempProperties) | ||
} | ||
if (onPendingChanges) { | ||
onPendingChanges(false) | ||
} | ||
setEditMode(false) | ||
} | ||
|
||
const handleCancel: () => void = () => { | ||
setTempDates({ | ||
dateFrom: dashboardFilters?.date_from, | ||
dateTo: dashboardFilters?.date_to, | ||
}) | ||
setTempProperties(dashboard?.filters.properties ?? undefined) | ||
setEditMode(false) | ||
if (onPendingChanges) { | ||
onPendingChanges(false) | ||
} | ||
} |
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.
This looks like it could be a logic. We do try to avoid react state as much as possible
Not sure I follow. I made it undead, added the stale. It shows up when you change filters so you don't wonder why nothing is happening. Do you mean this? |
# Conflicts: # frontend/__snapshots__/posthog-3000-navigation--navigation-3000--dark.png # frontend/__snapshots__/posthog-3000-navigation--navigation-3000--light.png # frontend/__snapshots__/posthog-3000-navigation--navigation-base--dark.png # frontend/__snapshots__/posthog-3000-navigation--navigation-base--light.png
Ignore me then 😬 |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Problem
I read about some confusion that adjusting the filters saves the insight for everyone in the team immediately.
Changes
Does this work well for both Cloud and self-hosted?
n/a
How did you test this code?