feat: set username behind flag #465
There are no files selected for viewing
|
There was a problem hiding this comment. name should be added to the |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,7 @@ subscribe(APP_READY, () => { | |
| authenticatedUser: { | ||
| userId: '123abc', | ||
| username: 'testuser', | ||
| name: 'test user', | ||
|
There was a problem hiding this comment. Another general question: Would we prefer this as There was a problem hiding this comment. The authenticated data sent in JWT has "name" not "fullName" |
||
| roles: [], | ||
| administrator: false, | ||
| }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -74,21 +74,25 @@ class DesktopHeader extends React.Component { | |||||
|
|
||||||
| renderUserMenu() { | ||||||
| const { | ||||||
| intl, | ||||||
| userMenu, | ||||||
| avatar, | ||||||
| name, | ||||||
|
There was a problem hiding this comment. I'll copy-paste from another review on a related PR: This would make so there's no longer a 1:1 relationship between the DesktopHeader props and what is actually shown. In other words, it's not immediately obvious without looking at the implementation what happens if you provide both username and name. In other words, unless I'm missing something, DesktopHeader has no reason to distinguish between |
||||||
| username, | ||||||
| } = this.props; | ||||||
| const hideUsername = getConfig().HIDE_USERNAME_FROM_HEADER; | ||||||
| const usernameOrName = hideUsername ? name : username; | ||||||
|
Comment on lines
+83
to
+84
There was a problem hiding this comment. It looks like this has been implemented in a way that assumes the "hide username from menu button in header" is tied to "don't use usernames." It seems to me those are two separate things. I could definitely see someone running an Open edX instance that uses usernames but not wanting to show them in the menu dropdown button. In this case the difference seems quite minor, but I can imagine scenarios where having "hide username" and "we don't use usernames" coupled could be problematic. There was a problem hiding this comment. We will be auto-generating username and showing that in alt text doesn't make sense. I can see how There was a problem hiding this comment. Maybe we can go with your suggestion of |
||||||
|
|
||||||
| return ( | ||||||
| <Menu transitionClassName="menu-dropdown" transitionTimeout={250}> | ||||||
| <MenuTrigger | ||||||
| tag="button" | ||||||
| aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })} | ||||||
|
There was a problem hiding this comment. Another open question: Do we want to use a new message key here? An alternative solution that comes to mind is
Suggested change
|
||||||
| className="btn btn-outline-primary d-inline-flex align-items-center pl-2 pr-3" | ||||||
| > | ||||||
| <Avatar size="1.5em" src={avatar} alt="" className="mr-2" /> | ||||||
|
There was a problem hiding this comment. I know this wasn't added in this PR, but StudioHeader uses name/username for the alt test in UserMenu. Does it make sense to do the same here for consistency? |
||||||
| {!hideUsername && username} | ||||||
| <CaretIcon role="img" aria-hidden focusable="false" /> | ||||||
| </MenuTrigger> | ||||||
| <MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2"> | ||||||
| {userMenu.map(({ type, href, content }) => ( | ||||||
|
|
@@ -178,6 +182,7 @@ DesktopHeader.propTypes = { | |||||
| logoDestination: PropTypes.string, | ||||||
| avatar: PropTypes.string, | ||||||
| username: PropTypes.string, | ||||||
| name: PropTypes.string, | ||||||
| loggedIn: PropTypes.bool, | ||||||
|
|
||||||
| // i18n | ||||||
|
|
@@ -207,6 +212,7 @@ DesktopHeader.defaultProps = { | |||||
| logoDestination: null, | ||||||
| avatar: null, | ||||||
| username: null, | ||||||
| name: null, | ||||||
| loggedIn: false, | ||||||
| appMenu: null, | ||||||
| }; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,6 +51,7 @@ const LearningHeader = ({ | |
| {showUserDropdown && authenticatedUser && ( | ||
| <AuthenticatedUserDropdown | ||
| username={authenticatedUser.username} | ||
| name={authenticatedUser.name} | ||
| /> | ||
| )} | ||
| {showUserDropdown && !authenticatedUser && ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import React from 'react'; | ||
| import { getConfig, mergeConfig } from '@edx/frontend-platform'; | ||
| import { | ||
| authenticatedUser, initializeMockApp, render, screen, | ||
| } from '../setupTest'; | ||
|
|
@@ -15,6 +16,17 @@ describe('Header', () => { | |
| expect(screen.getByText(authenticatedUser.username)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('displays user button without username', () => { | ||
| const config = getConfig(); | ||
| mergeConfig({ | ||
| ...config, | ||
| HIDE_USERNAME_FROM_HEADER: true, | ||
| }); | ||
| const { queryByTestId } = render(<Header />); | ||
| const userName = queryByTestId('username'); | ||
| expect(userName).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('displays course data', () => { | ||
| const courseData = { | ||
| courseOrg: 'course-org', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ const HeaderBody = ({ | |
| number, | ||
| org, | ||
| title, | ||
| name, | ||
| username, | ||
| isAdmin, | ||
| studioBaseUrl, | ||
|
|
@@ -99,6 +100,7 @@ const HeaderBody = ({ | |
| <Nav> | ||
| <UserMenu | ||
| {...{ | ||
| name, | ||
| username, | ||
| studioBaseUrl, | ||
| logoutUrl, | ||
|
|
@@ -124,6 +126,7 @@ HeaderBody.propTypes = { | |
| logo: PropTypes.string, | ||
| logoAltText: PropTypes.string, | ||
| authenticatedUserAvatar: PropTypes.string, | ||
| name: PropTypes.string, | ||
| username: PropTypes.string, | ||
| isAdmin: PropTypes.bool, | ||
| isMobile: PropTypes.bool, | ||
|
|
@@ -149,6 +152,7 @@ HeaderBody.defaultProps = { | |
| org: '', | ||
| title: '', | ||
| authenticatedUserAvatar: null, | ||
| name: null, | ||
| username: null, | ||
| isAdmin: false, | ||
| isMobile: false, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ const StudioHeader = ({ | |
| number, | ||
| org, | ||
| title, | ||
| name: authenticatedUser?.name, | ||
| username: authenticatedUser?.username, | ||
| isAdmin: authenticatedUser?.administrator, | ||
| authenticatedUserAvatar: authenticatedUser?.avatar, | ||
|
|
||
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.
General question (hoping to hear what others think): Do we prefer this as
HIDEor asSHOW? I could be persuaded either way.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.
Come to think of it, I could even see this being non-boolean: something like
NAME_IN_MENU_BUTTONwith the options beingusername,fullName,none