site-kit-wp
site-kit-wp copied to clipboard
User menu does not show which account is connected
Feature Description
Before the unified dashboard, the SK user menu used to have the user's email address in the header which showed the address of the connected Google account. This is no longer shown which makes for a more consistent header with other Google products, although it makes it hard to tell which user is connected as this isn't even surfaced in the site health info.
Most Google products show the email address of the connected account on hover or otherwise in the user menu, so we should do the same.
Otherwise, for users who use the same photo for multiple Google accounts, it can be almost impossible to tell which one is connected.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The tooltip for the user menu should be updated to show the user's Google account for consistency with other Google services
- The tooltip text for the user menu should be updated to:
Google Account {Full Name} {google account email address}
- The
googlesitekit_profile
user option should be updated to store a newfull_name
property using the firstdisplayName
from the user'snames
returned by the People API (seeOAuth_Client::refresh_profile_data
)
Implementation Brief
- In
OAuth_Client::refresh_profile_data()
: Addnames
to thepersonFields
' value when fetching people data within the people.get() method. This will add a names array to the response. Add this property within theprofile->set()
params as per the AC, i.e.'full_name' => $response['names'][0]['displayName']
. - In
assets/js/googlesitekit/datastore/user/user-info.js
: Create agetFullName
selector similar togetPicture
. (Return theuser.full_name
property if it is defined.) - Currently, our
<Button>
component uses thearia-label
(ortitle
) of the Button as the Tooltip text. Modifyassets/js/components/Button.js
and create a newcustomizedTooltip
prop which can accept a React element and renders a Customised MUI Tooltip (which allows text on multiple lines) if that prop is passed. - In
assets/js/components/UserMenu.js
:- Use the new
getFullName
selector to fetch the user's full name. - Pass a react element to the new
customizedTooltip
prop of the<Button>
component to render the tooltip as per the AC.
- Use the new
- Port/refactor the simple
stories/user-menu.stories.js
into a new Storybook v6 story and ensure the customized tooltip shows correctly. Ensure the other Storybook v5 plugin header stories (stories/header.stories.js
) render the new tooltip correctly.
Test Coverage
- Update
OAuth_ClientTest::test_refresh_profile_data()
- Add tests for the new selector in
user-info.test.js
.
QA Brief
- Navigate to the Dashboard and hover over the User menu.
- Verify the tooltip appears as per the AC.
- Check the tooltip appears in the
Header
andUserMenu
stories.- Note, the
UserMenu
story will now appear underComponents
rather thanGlobal
.
- Note, the
- A note for QA:
- The full name will currently only appear on a new Site or after you reset Site Kit. This is not the long-term plan, see: #6003
- Test that old sites without the user's full name do not error:
- Sign in to a site using Site Kit before using a branch with this change (eg. sign in using version
1.85.0
). - Replace your Site Kit plugin with
develop
- Ensure no errors appear when accessing Site Kit.
- Note that your name will not appear as it was not requested in previous versions of Site Kit, but no error should appear.
- Sign in to a site using Site Kit before using a branch with this change (eg. sign in using version
Changelog entry
- Show user account info in user menu.
@felixarntz do you think we need UX input here?
Also, most (if not all?) Google services show the user's name as well (not just the email) but we only have the email. Should we update our Profile
to include the name too and show this if it's available? That way it would work for existing users too.
@aaemnnosttv I think it would be great to look up the name then too so we can display fully consistent information.
Regarding the user's avatar, I don't think we need to enforce it to be 80x80, IMO the way it currently is is fine. I would suggest we make this issue only about the tooltip content, to match the regular Google experience there.
Just to confirm, the ACs otherwise are only about the tooltip correct? That's a bit unclear to me now when I read them.
Regarding the user's avatar, I don't think we need to enforce it to be 80x80, IMO the way it currently is is fine. I would suggest we make this issue only about the tooltip content, to match the regular Google experience there.
@felixarntz this isn't suggesting we change the size in the header but what is shown in the dropdown.
![image](https://user-images.githubusercontent.com/1621608/187738409-6d4c55d5-f2d8-4e43-b7ef-4c83841540c0.png)
So as a whole the suggestion here is to update the tooltip content, and add the larger avatar, name, and email to the top of the body of the dropdown as above (without the ability to edit your avatar of course).
@aaemnnosttv Ah that makes potentially more sense, but I think it has a bit more UX implications. We don't 100% mirror the Google UI anyway, so I don't know if we really need to have that information within the dropdown content.
I'd prefer if we focused this issue only on the tooltip content, and this way we can build the foundation here to also store the account name and then display it in the tooltip. We could open a follow-up issue to further evaluate what you're suggesting above regarding the dropdown content. WDYT?
We don't 100% mirror the Google UI anyway
True, but I think this portion of it makes sense to mirror (as it's rather small) but also relevant to the connected user which isn't currently shown.
I don't know if we really need to have that information within the dropdown content.
IMO, the email should be there at the least – we used to have it in the SK header! I would say following established UX for this type of thing used across products would feel more intuitive than deviating from it as we are now (even if somewhat accidentally perhaps).
I'd prefer if we focused this issue only on the tooltip content, and this way we can build the foundation here to also store the account name and then display it in the tooltip. We could open a follow-up issue to further evaluate what you're suggesting above regarding the dropdown content. WDYT?
Sure, although it might be a bit easy to miss if only in the tooltip. I'll update the ACs here to be limited to the tooltip and open a follow up issue as you suggested.
Thanks @jimmymadon – just a few points:
- Since any authenticated user won't have the name set yet, let's only render that part if there is one
- There are some relevant tests for
test_refresh_profile_data
which should get updated here as well, so let's note this in the test coverage and perhaps bump the estimate
Thanks @aaemnnosttv - have updated the IB!
IB ✅
(@techanvil won't be able to look at this for a bit, but there's an issue with the PR—I left a review. If someone else can pick it up that'd be great and I can review again, otherwise I'll take a look at this Monday or Tuesday.)
Hi @tofumatt just noticed this is in execution but not assigned, are you still planning on picking this up as Tom is o.o.o? Thanks!
I'll give this a look since I did the IB and have some context. Will go through @tofumatt 's comments on the open PR.
My comment here is affecting any site that was set up before this change: https://github.com/google/site-kit-wp/pull/5946#pullrequestreview-1139827695
I'm guessing when you tested this you were using a site that had been reset and pulling this "full name" data, but for sites that haven't retrieved this data yet there will be an error:
It's because this line is new: https://github.com/google/site-kit-wp/blob/0def4c239441e78f2eef88da46891a30d119fe8e/includes/Core/Authentication/Clients/OAuth_Client.php#L516
So we'll want to check if $profile_data[ 'full_name' ]
is empty before accessing it, and account for a possible null
value throughout the code for sites that haven't retrieved it. I suppose we could also just set it as an empty string until it's updated, or if the value is empty force a profile refresh so it's immediately available. 🤔
QA Update ✅
- Verified on dev.
- Verified for new site as well as for old site.
- On older site full name not display as per QAB but it display when user reset plugin or connect any module.
- No errors showing on the dev branch.
- The tooltip text for the user menu should be updated to:
Google Account {Full Name} {google account email address}
Older site :
After connecting new module or resetting the plugin
⚠️ Approval
On review, the tooltip looks off with an empty space where the full name would be. In this case, there are 3 consecutive line breaks where the last one should only be there if there is a full name.
This should be very quick to correct with a follow up PR.
All good now 👍
Screenshots