From ef31ead7b590cfe3377dbd1c7deb4e79bebb7e09 Mon Sep 17 00:00:00 2001 From: mubbsharanwar <mubbshar.anwar@arbisoft.com> 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 +- src/__snapshots__/Header.test.jsx.snap | 2 - .../AuthenticatedUserDropdown.jsx | 16 ++- src/learning-header/LearningHeader.test.jsx | 6 +- src/studio-header/StudioHeader.test.jsx | 11 +- src/studio-header/UserMenu.jsx | 6 +- 10 files changed, 78 insertions(+), 105 deletions(-) diff --git a/README.rst b/README.rst index eba016a348..b6f7c5ae91 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_FROM_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..725eaab372 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 ( + <a key={`${type}-${content}`} className="nav-link" href={href}>{content}</a> + ); + } + + return ( + <Menu key={`${type}-${content}`} tag="div" className="nav-item" respondToPointerEvents> + <MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center" href={href}> + {content} <CaretIcon role="img" aria-hidden focusable="false" /> + </MenuTrigger> + <MenuContent className="pin-left pin-right shadow py-2"> + {submenuContent} + </MenuContent> + </Menu> + ); + }); + } + + // Renders an optional App Menu for + renderAppMenu() { + const { appMenu } = this.props; + const { content: appMenuContent, menuItems } = appMenu; return ( <Menu transitionClassName="menu-dropdown" transitionTimeout={250}> - <MenuTrigger - tag="button" - aria-label={intl.formatMessage(messages['header.label.account.menu.with.username'], { username })} - 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" /> - {username} <CaretIcon role="img" aria-hidden focusable="false" /> + <MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center"> + {appMenuContent} <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 }) => ( + {menuItems.map(({ type, href, content }) => ( <a className={`dropdown-${type}`} key={`${type}-${content}`} href={href}>{content}</a> ))} </MenuContent> @@ -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_FROM_HEADER; + const usernameOrName = hideUsername ? name : username; return ( <Menu transitionClassName="menu-dropdown" transitionTimeout={250}> <MenuTrigger tag="button" - aria-label={intl.formatMessage(messages['header.label.account.menu.without.username'], { name })} + aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })} 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" /> + {hideUsername ? null : username} <CaretIcon role="img" aria-hidden focusable="false" /> </MenuTrigger> <MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2"> @@ -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 ( - <Menu transitionClassName="menu-dropdown" transitionTimeout={250}> - <MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center"> - {appMenuContent} <CaretIcon role="img" aria-hidden focusable="false" /> - </MenuTrigger> - <MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2"> - {menuItems.map(({ type, href, content }) => ( - <a className={`dropdown-${type}`} key={`${type}-${content}`} href={href}>{content}</a> - ))} - </MenuContent> - </Menu> - ); - } - - 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 ( - <a key={`${type}-${content}`} className="nav-link" href={href}>{content}</a> - ); - } - - return ( - <Menu key={`${type}-${content}`} tag="div" className="nav-item" respondToPointerEvents> - <MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center" href={href}> - {content} <CaretIcon role="img" aria-hidden focusable="false" /> - </MenuTrigger> - <MenuContent className="pin-left pin-right shadow py-2"> - {submenuContent} - </MenuContent> - </Menu> - ); - }); - } - renderLoggedOutItems() { const { loggedOutItems } = this.props; diff --git a/src/Header.jsx b/src/Header.jsx index efc981d12f..cd40126ea9 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_FROM_HEADER: !!process.env.HIDE_USERNAME_FROM_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..b4f09f61e8 100644 --- a/src/Header.test.jsx +++ b/src/Header.test.jsx @@ -76,7 +76,7 @@ describe('<Header />', () => { 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_FROM_HEADER: true, }, }; const component = <HeaderComponent width={{ width: 1280 }} contextValue={contextValue} />; diff --git a/src/__snapshots__/Header.test.jsx.snap b/src/__snapshots__/Header.test.jsx.snap index be5cb8b467..04aeaf59a2 100644 --- a/src/__snapshots__/Header.test.jsx.snap +++ b/src/__snapshots__/Header.test.jsx.snap @@ -281,7 +281,6 @@ exports[`<Header /> renders correctly for authenticated desktop with username 1` </svg> </span> edX - <svg aria-hidden={true} focusable="false" @@ -389,7 +388,6 @@ exports[`<Header /> renders correctly for authenticated desktop without username /> </svg> </span> - <svg aria-hidden={true} focusable="false" diff --git a/src/learning-header/AuthenticatedUserDropdown.jsx b/src/learning-header/AuthenticatedUserDropdown.jsx index 9055b7eb70..af25012d9a 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 = ( <Dropdown.Item href={`${getConfig().LMS_BASE_URL}/dashboard`}> @@ -19,14 +18,17 @@ const AuthenticatedUserDropdown = ({ </Dropdown.Item> ); - const showDropdownToggle = ( + // show avatar instead username if HIDE_USERNAME_FROM_HEADER flag is enabled + const dropdownToggle = ( <Dropdown.Toggle variant="outline-primary"> <FontAwesomeIcon icon={faUserCircle} className="d-md-none" size="lg" /> - {!getConfig().ENABLE_HEADER_WITHOUT_USERNAME ? ( + {getConfig().HIDE_USERNAME_FROM_HEADER ? ( + <Avatar size="sm" src={avatar} alt={name} className="mr-2" /> + ) : ( <span data-hj-suppress className="d-none d-md-inline" data-testid="username"> {username} </span> - ) : <Avatar size="sm" src={avatar} alt={name} className="mr-2" />} + )} </Dropdown.Toggle> ); @@ -34,7 +36,7 @@ const AuthenticatedUserDropdown = ({ <> <a className="text-gray-700" href={`${getConfig().SUPPORT_URL}`}>{intl.formatMessage(messages.help)}</a> <Dropdown className="user-dropdown ml-3"> - {showDropdownToggle} + {dropdownToggle} <Dropdown.Menu className="dropdown-menu-right"> {dashboardMenuItem} <Dropdown.Item href={`${getConfig().ACCOUNT_PROFILE_URL}/u/${username}`}> diff --git a/src/learning-header/LearningHeader.test.jsx b/src/learning-header/LearningHeader.test.jsx index f004124ad7..64029815e5 100644 --- a/src/learning-header/LearningHeader.test.jsx +++ b/src/learning-header/LearningHeader.test.jsx @@ -20,10 +20,10 @@ describe('Header', () => { const config = getConfig(); mergeConfig({ ...config, - ENABLE_HEADER_WITHOUT_USERNAME: true, + HIDE_USERNAME_FROM_HEADER: true, }); - const { queryByText } = render(<Header />); - const userName = queryByText(config.authenticatedUser.username); + const { queryByTestId } = render(<Header />); + const userName = queryByTestId('username'); expect(userName).not.toBeInTheDocument(); }); diff --git a/src/studio-header/StudioHeader.test.jsx b/src/studio-header/StudioHeader.test.jsx index 7affc3f91b..6412060f5c 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(<RootWrapper {...newProps} />); + it('user menu should not contain username', async () => { + const config = getConfig(); + mergeConfig({ + ...config, + HIDE_USERNAME_FROM_HEADER: true, + }); + const { container } = render(<RootWrapper {...props} />); 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..6d6b1f3675 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_FROM_HEADER; + const avatarAlt = hideUsername ? name : username; const avatar = authenticatedUserAvatar ? ( <img className="d-block w-100 h-100" @@ -36,7 +36,7 @@ const UserMenu = ({ data-testid="avatar-icon" /> ); - const title = isMobile ? avatar : <>{avatar}{showUsername && username}</>; + const title = (isMobile || hideUsername) ? avatar : <>{avatar}{username}</>; return ( <NavDropdownMenu