-
Notifications
You must be signed in to change notification settings - Fork 159
feat: Move home bookmark to a separate button on the top bar #6979
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
Conversation
24e6f9c to
f593189
Compare
ericpgreen2
left a comment
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.
Let's get a Product review from @jkhwu. A few UXQA I see:
- The Home button does not have the same hover state as the buttons it's next to
- Should the Home button be disabled when either there's no Home bookmark or you're currently viewing the Home state? Disabled + tooltip to explain why.
- It feels a bit off that the "Bookmark your current view as home" action exists in a dropdown that's anchored off a different button. Maybe, when there's no Home bookmark, the Home button should open a dropdown menu with the prompt?(CC @jkhwu)
Looks like the Home bookmark e2e test needs to be updated.
|
Thanks @ericpgreen2
|
|
Code looks good. I'll defer to @mindspank to approve as product owner, as I have reservations about this:
|
ericpgreen2
left a comment
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.
Design-wise, IMO we need to make clear the difference between these two menu items. They look "equal", yet one should be clearly a navigation action and one should clearly be not. A couple ideas:
- Adding a separator between the menu items
- Editing the "Home" copy to be "Go to home"
- Moving the "Go to home" menu item above the other, since it'll be the more common action.
@jkhwu, any other thoughts on this?
| export let gray = false; | ||
| export let danger = false; | ||
| export let preload = true; | ||
| export let highlight = 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.
We should use the same term from Figma: "active"
0e4df1d to
aa4a966
Compare
* Move home bookmark to a separate button * Fix url not updating state * Cleanup * Update tests * Add active/inactive states * Add home bookmark dropdown * Fix E2E * Update UX * Add tooltip for delete home bookmark
Figure out what we will show on a shared url and rill-dev.We will not show this for shared-url and rill-dev.Checklist: