fix: address reviewer comments

openedx · Feb 9, 2024 · ef31ead · ef31ead
1 parent 4cc31ca
commit ef31ead
Show file tree

Hide file tree

Showing 10 changed files with 78 additions and 105 deletions.
diff --git 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
@@ -19,49 +19,79 @@ 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>
       </Menu>
     );
   }
 
-  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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -4,37 +4,39 @@ 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`}>
       {intl.formatMessage(messages.dashboard)}
     </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>
   );
 
   return (
     <>
       <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
@@ -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
@@ -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
@@ -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
-Original file line number
+Diff line change
@@ Expand Up @@
                   </svg>
                 </span>
                 edX
                 <svg
                   aria-hidden={true}
                   focusable="false"
@@ Expand Down Expand Up @@
                     />
                   </svg>
                 </span>
                 <svg
                   aria-hidden={true}
                   focusable="false"
@@ Expand Down @@