-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Dashboards: allow changing date filters & refresh #3363
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.
Looks good overall. a small note
const [rangeDateFrom, setRangeDateFrom] = useState( | ||
dateFrom && isDate.test(dateFrom as string) ? moment(dateFrom) : undefined | ||
) | ||
const [rangeDateTo, setRangeDateTo] = useState(dateTo && isDate.test(dateTo as string) ? moment(dateTo) : undefined) |
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.
Can also accept these as moments directly since the dateFrom/dateTo are typed as undefined | string | moment
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 remember there being a good reason why these useState
calls needed to exist, but it's not obvious from code. They were also there in the previous version btw. :/
const { dashboard, itemsLoading, items } = useValues(logic) | ||
function _Dashboard({ id, shareToken }: Props): JSX.Element { | ||
return ( | ||
<BindLogic logic={dashboardLogic} props={{ id: parseInt(id), shareToken }}> |
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.
💪
} | ||
actions.updateDashboard(filters) | ||
dashboardItemsModel.actions.refreshAllDashboardItems(filters) | ||
}, | ||
}), | ||
}) |
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.
One inconsistency I found was that if you had set a dashboard date filter, refreshing the page doesn't apply the filter even though it's in the url params
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.
Interesting, it should not be part of the URL anymore but be saved on the model. Were you on the latest branch?
if (props.dashboardItemId) { | ||
actions.setFilters(filters) | ||
} | ||
}, | ||
}), | ||
}) |
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.
Not quite relevant here, but important to note is that when using a non custom date range (where dateto defaults to now()), retention dashboard items won't have any change which could be confusing UX-wise.
(datefilters on dashboard)
Previously funnelLogic might be mounted in the background, causing a reload
ad4b270
to
e88cf55
Compare
7c83c6f
to
7c756e8
Compare
Changes
After this change we show a date filter + refresh button in the dashboard headers.
On a high level dashboards now have a separate filters property which needs to be merged into dashboard items.
This was a deceptively hard change. Some things which got refactored as part of this:
Next steps
Since this PR is huge, I've split off some work:
Checklist