From dd978ecac865e7a7fff556676041443a1efc9bef Mon Sep 17 00:00:00 2001 From: mubbsharanwar Date: Wed, 7 Feb 2024 13:50:16 +0500 Subject: [PATCH] fix: address reviewer comments --- README.rst | 2 +- src/DesktopHeader.jsx | 123 +++++++----------- src/Header.jsx | 2 +- src/Header.messages.jsx | 13 +- src/Header.test.jsx | 2 +- .../AuthenticatedUserDropdown.jsx | 16 ++- src/learning-header/LearningHeader.test.jsx | 2 +- src/studio-header/StudioHeader.test.jsx | 11 +- src/studio-header/UserMenu.jsx | 6 +- 9 files changed, 76 insertions(+), 101 deletions(-) diff --git a/README.rst b/README.rst index eba016a348..ecccd61c4b 100644 --- a/README.rst +++ b/README.rst @@ -49,7 +49,7 @@ Environment Variables * ``ACCOUNT_PROFILE_URL`` - The URL of the account profile page. * ``ACCOUNT_SETTINGS_URL`` - The URL of the account settings page. * ``AUTHN_MINIMAL_HEADER`` - A boolean flag which hides the main menu, user menu, and logged-out -* ``ENABLE_HEADER_WITHOUT_USERNAME`` - A boolean flag which hides the username from the header +* ``HIDE_USERNAME_IN_HEADER`` - A boolean flag which hides the username from the header menu items when truthy. This is intended to be used in micro-frontends like frontend-app-authentication in which these menus are considered distractions from the user's task. diff --git a/src/DesktopHeader.jsx b/src/DesktopHeader.jsx index 055afd82d5..b9c1f0f622 100644 --- a/src/DesktopHeader.jsx +++ b/src/DesktopHeader.jsx @@ -19,26 +19,52 @@ class DesktopHeader extends React.Component { super(props); } - userMenuWithUsername() { - const { - userMenu, - avatar, - username, - intl, - } = this.props; + renderMainMenu() { + const { mainMenu } = this.props; + // Nodes are accepted as a prop + if (!Array.isArray(mainMenu)) { + return mainMenu; + } + + return mainMenu.map((menuItem) => { + const { + type, + href, + content, + submenuContent, + } = menuItem; + + if (type === 'item') { + return ( + {content} + ); + } + + return ( + + + {content} + + + {submenuContent} + + + ); + }); + } + + // Renders an optional App Menu for + renderAppMenu() { + const { appMenu } = this.props; + const { content: appMenuContent, menuItems } = appMenu; return ( - - - {username} + + {appMenuContent} - {userMenu.map(({ type, href, content }) => ( + {menuItems.map(({ type, href, content }) => ( {content} ))} @@ -46,22 +72,26 @@ class DesktopHeader extends React.Component { ); } - userMenuWithoutUsername() { + renderUserMenu() { const { + intl, userMenu, avatar, name, - intl, + username, } = this.props; + const hideUsername = getConfig().HIDE_USERNAME_IN_HEADER; + const usernameOrName = hideUsername ? name : username; return ( + {hideUsername ? null : username} @@ -73,63 +103,6 @@ class DesktopHeader extends React.Component { ); } - renderUserMenu() { - return (getConfig().ENABLE_HEADER_WITHOUT_USERNAME ? this.userMenuWithoutUsername() : this.userMenuWithUsername()); - } - - // Renders an optional App Menu for - renderAppMenu() { - const { appMenu } = this.props; - const { content: appMenuContent, menuItems } = appMenu; - return ( - - - {appMenuContent} - - - {menuItems.map(({ type, href, content }) => ( - {content} - ))} - - - ); - } - - renderMainMenu() { - const { mainMenu } = this.props; - - // Nodes are accepted as a prop - if (!Array.isArray(mainMenu)) { - return mainMenu; - } - - return mainMenu.map((menuItem) => { - const { - type, - href, - content, - submenuContent, - } = menuItem; - - if (type === 'item') { - return ( - {content} - ); - } - - return ( - - - {content} - - - {submenuContent} - - - ); - }); - } - renderLoggedOutItems() { const { loggedOutItems } = this.props; diff --git a/src/Header.jsx b/src/Header.jsx index efc981d12f..016ddcb2a2 100644 --- a/src/Header.jsx +++ b/src/Header.jsx @@ -27,7 +27,7 @@ ensureConfig([ subscribe(APP_CONFIG_INITIALIZED, () => { mergeConfig({ AUTHN_MINIMAL_HEADER: !!process.env.AUTHN_MINIMAL_HEADER, - ENABLE_HEADER_WITHOUT_USERNAME: !!process.env.ENABLE_HEADER_WITHOUT_USERNAME, + HIDE_USERNAME_IN_HEADER: !!process.env.HIDE_USERNAME_IN_HEADER, }, 'Header additional config'); }); diff --git a/src/Header.messages.jsx b/src/Header.messages.jsx index 6db83e5c73..1071c9cb37 100644 --- a/src/Header.messages.jsx +++ b/src/Header.messages.jsx @@ -76,15 +76,10 @@ const messages = defineMessages({ defaultMessage: 'Account Menu', description: 'The aria label for the account menu trigger', }, - 'header.label.account.menu.with.username': { - id: 'header.label.account.menu.with.username', - defaultMessage: 'Account menu for {username}', - description: 'The aria label for the account menu trigger when the username is displayed in it', - }, - 'header.label.account.menu.without.username': { - id: 'header.label.account.without.username', - defaultMessage: 'Account menu for {name}', - description: 'The aria label for the account menu trigger when ENABLE_HEADER_WITHOUT_USERNAME is enabled', + 'header.label.account.menu.using.name.or.username': { + id: 'header.label.account.menu.using.name.or.username', + defaultMessage: 'Account menu for {usernameOrName}', + description: 'The aria label for the account menu trigger when the username is displayed or hide in it', }, 'header.label.main.nav': { id: 'header.label.main.nav', diff --git a/src/Header.test.jsx b/src/Header.test.jsx index ff0e33c307..0fef66f105 100644 --- a/src/Header.test.jsx +++ b/src/Header.test.jsx @@ -76,7 +76,7 @@ describe('
', () => { LOGIN_URL: process.env.LOGIN_URL, LOGOUT_URL: process.env.LOGOUT_URL, LOGO_URL: process.env.LOGO_URL, - ENABLE_HEADER_WITHOUT_USERNAME: true, + HIDE_USERNAME_IN_HEADER: true, }, }; const component = ; diff --git a/src/learning-header/AuthenticatedUserDropdown.jsx b/src/learning-header/AuthenticatedUserDropdown.jsx index 9055b7eb70..343f312287 100644 --- a/src/learning-header/AuthenticatedUserDropdown.jsx +++ b/src/learning-header/AuthenticatedUserDropdown.jsx @@ -4,14 +4,13 @@ import PropTypes from 'prop-types'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; import { faUserCircle } from '@fortawesome/free-solid-svg-icons'; import { getConfig } from '@edx/frontend-platform'; -import { Avatar } from '@openedx/paragon'; import { injectIntl, intlShape } from '@edx/frontend-platform/i18n'; -import { Dropdown } from '@openedx/paragon'; +import { Avatar, Dropdown } from '@openedx/paragon'; import messages from './messages'; const AuthenticatedUserDropdown = ({ - intl, username, avatar, name + intl, username, avatar, name, }) => { const dashboardMenuItem = ( @@ -19,14 +18,17 @@ const AuthenticatedUserDropdown = ({ ); - const showDropdownToggle = ( + // show avatar instead username if HIDE_USERNAME_IN_HEADER flag is enabled + const dropdownToggle = ( - {!getConfig().ENABLE_HEADER_WITHOUT_USERNAME ? ( + {getConfig().HIDE_USERNAME_IN_HEADER ? ( + + ) : ( {username} - ) : } + )} ); @@ -34,7 +36,7 @@ const AuthenticatedUserDropdown = ({ <> {intl.formatMessage(messages.help)} - {showDropdownToggle} + {dropdownToggle} {dashboardMenuItem} diff --git a/src/learning-header/LearningHeader.test.jsx b/src/learning-header/LearningHeader.test.jsx index f004124ad7..43eb114283 100644 --- a/src/learning-header/LearningHeader.test.jsx +++ b/src/learning-header/LearningHeader.test.jsx @@ -20,7 +20,7 @@ describe('Header', () => { const config = getConfig(); mergeConfig({ ...config, - ENABLE_HEADER_WITHOUT_USERNAME: true, + HIDE_USERNAME_IN_HEADER: true, }); const { queryByText } = render(
); const userName = queryByText(config.authenticatedUser.username); diff --git a/src/studio-header/StudioHeader.test.jsx b/src/studio-header/StudioHeader.test.jsx index 7affc3f91b..5cf33aa601 100644 --- a/src/studio-header/StudioHeader.test.jsx +++ b/src/studio-header/StudioHeader.test.jsx @@ -1,5 +1,6 @@ /* eslint-disable react/prop-types */ import React, { useMemo } from 'react'; +import { getConfig, mergeConfig } from '@edx/frontend-platform'; import { render, fireEvent, @@ -128,9 +129,13 @@ describe('Header', () => { expect(avatarIcon).toBeVisible(); }); - it.only('user menu should not contain username', async () => { - const newProps = { ...props, ENABLE_HEADER_WITHOUT_USERNAME: true }; - const { container } = render(); + it('user menu should not contain username', async () => { + const config = getConfig(); + mergeConfig({ + ...config, + HIDE_USERNAME_IN_HEADER: true, + }); + const { container } = render(); const userMenue = container.querySelector('#user-dropdown-menu'); expect(userMenue.textContent).toContain(''); }); diff --git a/src/studio-header/UserMenu.jsx b/src/studio-header/UserMenu.jsx index 69927fbe08..24a6c5a0c1 100644 --- a/src/studio-header/UserMenu.jsx +++ b/src/studio-header/UserMenu.jsx @@ -19,8 +19,8 @@ const UserMenu = ({ // injected intl, }) => { - const showUsername = !getConfig().ENABLE_HEADER_WITHOUT_USERNAME; - const avatarAlt = showUsername ? username : name; + const hideUsername = getConfig().HIDE_USERNAME_IN_HEADER; + const avatarAlt = hideUsername ? name : username; const avatar = authenticatedUserAvatar ? ( ); - const title = isMobile ? avatar : <>{avatar}{showUsername && username}; + const title = (isMobile || hideUsername) ? avatar : <>{avatar}{username}; return (