feat: User profile + settings menu in top right (AppHeader) instead of top/bottom left (AppSidebar)#6793
Conversation
13ab15f to
2a9eb3c
Compare
| v-model="sidebar" | ||
| absolute | ||
| :top-link="topLinks" | ||
| :user="{ data: true }" |
There was a problem hiding this comment.
comment: it seems this user prop of the sidebar wasn't even used anymore before my changes in this PR
| <!-- TODO: what was the use case for sidebar settings in logged-out state? --> | ||
| <v-divider v-if="loggedIn" class="my-2" /> | ||
| <v-list-item v-if="loggedIn" :prepend-icon="$globals.icons.cog" :title="$t('profile.user-settings')" to="/user/profile" /> |
There was a problem hiding this comment.
comment: right now, these v-if="loggedIn" conditionals here are redundant, because the entire user menu is only rendered when a user is logged in.
If we want to show a menu for logged-out users too (e.g. for switching language and theme), this needs to be reviews. If we don't want to support that use-case, these redundant conditionals should be removed. See PR description for more details.
|
The E2E tests are currently failing because they expect the user name to appear in the sidebar (which it now no longer does). I'm looking into how to adjust them to find the user name in the user menu instead. |
2a9eb3c to
f2c543f
Compare
|
I've updated the E2E tests to work locally. Edit: and they work in CI as well 👌 |
|
That sounds good! Here are a few suggestions:
|
|
@p0lycarpio, thanks for your feedback! 🙌
Ah yes, good point. While looking around at other examples of this pattern, I originally thought some other sites put the profile image into the popover menu, and use that to link to the profile page. Looking around again now, that doesn't seem to be the case. Some examples:
Oh, good catch! I didn't even realize that "User Settings" links to the same page as the "Profile" link (what used to be the link of the profile image on the top left). This is clearly redundant. In Mealie, the so-called "profile" page (at Combining this with your first suggestion, I think I'll go for something like "Account" or "My Account" (though this may introduce a new translation string, I'll have to check if there is already a suitable one).
Fair point, I didn't really reflect on this too much (since I never use this). But adjusting the order based on further feedback should be easy, so I'll move it over there for the time being.
Agreed that this doesn't need to be here (similar to the old Logout button in the top bar, the language switcher is likely too prominent here). |
…f top/bottom left (AppSidebar)
f2c543f to
230c06f
Compare
| { | ||
| icon: $globals.icons.heart, | ||
| // TODO: how to hide this for non-logged-in view? | ||
| to: `/user/${$auth.user.value.id}/favorites`, | ||
| title: i18n.t("user.favorite-recipes"), | ||
| restricted: false, | ||
| }, |
There was a problem hiding this comment.
comment: this change now introduced a dependency of this file to the logged-in state of the user; how should we handle this?
There was a problem hiding this comment.
You can set restricted to true for this.
p0lycarpio
left a comment
There was a problem hiding this comment.
Thanks for the changes, it looks good! Here are a few more suggestions for the layout and spacing.
| { | ||
| icon: $globals.icons.heart, | ||
| // TODO: how to hide this for non-logged-in view? | ||
| to: `/user/${$auth.user.value.id}/favorites`, | ||
| title: i18n.t("user.favorite-recipes"), | ||
| restricted: false, | ||
| }, |
There was a problem hiding this comment.
You can set restricted to true for this.
| </v-icon> | ||
| {{ smAndUp ? $t("user.logout") : "" }} | ||
| </v-btn> | ||
| <div v-if="loggedIn && sessionUser" class="mx-1"> |
There was a problem hiding this comment.
<div v-if="loggedIn && sessionUser" :class="xs ? 'pr-2' : 'px-2'">
| <template v-if="xs"> | ||
| <v-list-subheader :title="sessionUser.fullName ?? undefined" class="justify-center" /> | ||
| <v-divider class="my-2" /> | ||
| </template> |
There was a problem hiding this comment.
Instead of the block template above, here is another way to do this on mobile:
<v-list-item
:prepend-icon="$globals.icons.user"
:title="$t('settings.profile')"
:subtitle="xs ? sessionUser.fullName || undefined : undefined"
:to="userProfileLink"
:lines="xs ? 'two' : 'one'"
/>
michael-genson
left a comment
There was a problem hiding this comment.
Generally like this change, I think it makes sense and solves the log out issue. A few notes:
- You can browse Mealie even if you aren't logged-in, so keeping language and theme settings available is important
- I don't think we should get rid of the setting menu, putting it in the lower left seems natural to me. Can we instead limit the new profile button to just "Profile" and "Logout" and keep the rest in settings?
- This might just be personal opinion but the button being rounded looks a little odd to me since the profile picture is already rounded and right up against the border, maybe we can make it less round and add some padding? Open to criticism on this, I'm more of a backend person



What this PR does / why we need it:
(REQUIRED)
This PR consolidates the link to the current user's profile and the "settings" submenu into a single profile/account menu, represented by the user's avatar in the top right (i.e. at the end of the AppHeader).
This aligns Mealie's UI more with common conventions found in other apps that have user accounts (e.g. Github, Google, etc.).
This also prevents accidental logout, especially on mobile, where the logout button on the top right was easy to hit by mistake (and there was no confirmation dialog before logging out).
In more detail:
Screenshots
(menu closed)
(menu open)
Which issue(s) this PR fixes:
(REQUIRED)
Addresses discussion #5528
Special notes for your reviewer:
There are three things in particular I'd like reviewers to take a look at and/or let me know:
Two minor things I noticed but didn't address in this PR yet (i think these can be addressed in follow-up PRs if desired):
Inside the new profile menu, the user avatar and the other icons are not horizontally centered (they're left-aligned). This is the default behavior of Vuetify'sThis is now addressed -- the user avatar is no longer showing inside of the menuv-list, and I didn't find an obvious way to change this. In fact,v-list-itemuses pretty hard-coded spacing between the prepend slot and the item's text, making customization of this a bit tricky (and probably not easy to maintain).Testing
Manually, running the dev server locally.