-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Dashboard item page view #4142
Dashboard item page view #4142
Conversation
(cherry picked from commit 63cec8e6c9416a3d89a0420894c9184dec36256d)
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.
Nice stuff! Code-wise looks pretty solid, just made a comment on the way we pass params in the query string and one nit comment. Pushed a very minor commit with some comment clarifications, seemed faster than adding a review comment.
Functionality wise just found a problem. When you open a saved insight from the History tab, it shows like this (as in it says "On dashboard" but it's not actually part of a dashboard).
UI wise we had this design, sorry if we missed linking it in the original issue, which I think would be pretty nice to add (let me know if you want me to help here). One small change, I do love what you did of replacing the "Insights" title with the name of the item, I would keep that.
Closing while I implement changes :D |
Not sure if you meant for review yet but was taking a look at the implementation and leaving my 2 🪙. Overall looks great only 2 points for a UI perspective
Great work so far |
return `/insights/${id}` | ||
} | ||
|
||
export const displayHistoryItemLink = ({ id, dashboard, filters }: DashboardItemType): string => { |
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.
It was a bit of a pain to reroute the Insights History with the new insights/${id}
links so I'm leaving it as it was originally (for now) to maintain functionality.
Updates include:
|
Hey @liyiy! Let me know if you want me to re-review |
My PR is getting huge, so I would like to clean/touch up what I have now if it's working fine and then open a new PR to make more updates 😅 Otherwise feel free to lay any suggestions if the high level approach doesn't seem correct |
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.
Honestly this looks amazing! A few comments but nothing blocking. Feel free to address what you think makes sense.
UX wise I did notice an issue.
- When visiting an insight from the history panel, the back button will not work as expected (as we do some URL changes that add an extra step to the history state). You would have to click back twice for it to work.
Changes
Updating the dashboard item view page first before adding edit mode and then history panel for #3551
When you click on a dashboard item from the dashboards page, it's now clearer that it's a dedicated dashboard item page
BEFORE:
AFTER:
Checklist