Skip to content

Commit dbda420

Browse files
mubbsharanwarmubbsharanwar
committed
fix: address reviewer comments
1 parent 4cc31ca commit dbda420

10 files changed

+76
-103
lines changed

‎README.rst

+1-1
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ Environment Variables
4949
* ``ACCOUNT_PROFILE_URL`` - The URL of the account profile page.
5050
* ``ACCOUNT_SETTINGS_URL`` - The URL of the account settings page.
5151
* ``AUTHN_MINIMAL_HEADER`` - A boolean flag which hides the main menu, user menu, and logged-out
52-
* ``ENABLE_HEADER_WITHOUT_USERNAME`` - A boolean flag which hides the username from the header
52+
* ``HIDE_USERNAME_IN_HEADER`` - A boolean flag which hides the username from the header
5353
menu items when truthy. This is intended to be used in micro-frontends like
5454
frontend-app-authentication in which these menus are considered distractions from the user's task.
5555

‎src/DesktopHeader.jsx

+48-75
Original file line numberDiff line numberDiff line change
@@ -19,49 +19,79 @@ class DesktopHeader extends React.Component {
1919
super(props);
2020
}
2121

22-
userMenuWithUsername() {
23-
const {
24-
userMenu,
25-
avatar,
26-
username,
27-
intl,
28-
} = this.props;
22+
renderMainMenu() {
23+
const { mainMenu } = this.props;
2924

25+
// Nodes are accepted as a prop
26+
if (!Array.isArray(mainMenu)) {
27+
return mainMenu;
28+
}
29+
30+
return mainMenu.map((menuItem) => {
31+
const {
32+
type,
33+
href,
34+
content,
35+
submenuContent,
36+
} = menuItem;
37+
38+
if (type === 'item') {
39+
return (
40+
<a key={`${type}-${content}`} className="nav-link" href={href}>{content}</a>
41+
);
42+
}
43+
44+
return (
45+
<Menu key={`${type}-${content}`} tag="div" className="nav-item" respondToPointerEvents>
46+
<MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center" href={href}>
47+
{content} <CaretIcon role="img" aria-hidden focusable="false" />
48+
</MenuTrigger>
49+
<MenuContent className="pin-left pin-right shadow py-2">
50+
{submenuContent}
51+
</MenuContent>
52+
</Menu>
53+
);
54+
});
55+
}
56+
57+
// Renders an optional App Menu for
58+
renderAppMenu() {
59+
const { appMenu } = this.props;
60+
const { content: appMenuContent, menuItems } = appMenu;
3061
return (
3162
<Menu transitionClassName="menu-dropdown" transitionTimeout={250}>
32-
<MenuTrigger
33-
tag="button"
34-
aria-label={intl.formatMessage(messages['header.label.account.menu.with.username'], { username })}
35-
className="btn btn-outline-primary d-inline-flex align-items-center pl-2 pr-3"
36-
>
37-
<Avatar size="1.5em" src={avatar} alt="" className="mr-2" />
38-
{username} <CaretIcon role="img" aria-hidden focusable="false" />
63+
<MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center">
64+
{appMenuContent} <CaretIcon role="img" aria-hidden focusable="false" />
3965
</MenuTrigger>
4066
<MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2">
41-
{userMenu.map(({ type, href, content }) => (
67+
{menuItems.map(({ type, href, content }) => (
4268
<a className={`dropdown-${type}`} key={`${type}-${content}`} href={href}>{content}</a>
4369
))}
4470
</MenuContent>
4571
</Menu>
4672
);
4773
}
4874

49-
userMenuWithoutUsername() {
75+
renderUserMenu() {
5076
const {
77+
intl,
5178
userMenu,
5279
avatar,
5380
name,
54-
intl,
81+
username,
5582
} = this.props;
83+
const hideUsername = getConfig().HIDE_USERNAME_IN_HEADER;
84+
const usernameOrName = hideUsername ? name : username;
5685

5786
return (
5887
<Menu transitionClassName="menu-dropdown" transitionTimeout={250}>
5988
<MenuTrigger
6089
tag="button"
61-
aria-label={intl.formatMessage(messages['header.label.account.menu.without.username'], { name })}
90+
aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })}
6291
className="btn btn-outline-primary d-inline-flex align-items-center pl-2 pr-3"
6392
>
6493
<Avatar size="1.5em" src={avatar} alt="" className="mr-2" />
94+
{hideUsername ? null : username}
6595
<CaretIcon role="img" aria-hidden focusable="false" />
6696
</MenuTrigger>
6797
<MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2">
@@ -73,63 +103,6 @@ class DesktopHeader extends React.Component {
73103
);
74104
}
75105

76-
renderUserMenu() {
77-
return (getConfig().ENABLE_HEADER_WITHOUT_USERNAME ? this.userMenuWithoutUsername() : this.userMenuWithUsername());
78-
}
79-
80-
// Renders an optional App Menu for
81-
renderAppMenu() {
82-
const { appMenu } = this.props;
83-
const { content: appMenuContent, menuItems } = appMenu;
84-
return (
85-
<Menu transitionClassName="menu-dropdown" transitionTimeout={250}>
86-
<MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center">
87-
{appMenuContent} <CaretIcon role="img" aria-hidden focusable="false" />
88-
</MenuTrigger>
89-
<MenuContent className="mb-0 dropdown-menu show dropdown-menu-right pin-right shadow py-2">
90-
{menuItems.map(({ type, href, content }) => (
91-
<a className={`dropdown-${type}`} key={`${type}-${content}`} href={href}>{content}</a>
92-
))}
93-
</MenuContent>
94-
</Menu>
95-
);
96-
}
97-
98-
renderMainMenu() {
99-
const { mainMenu } = this.props;
100-
101-
// Nodes are accepted as a prop
102-
if (!Array.isArray(mainMenu)) {
103-
return mainMenu;
104-
}
105-
106-
return mainMenu.map((menuItem) => {
107-
const {
108-
type,
109-
href,
110-
content,
111-
submenuContent,
112-
} = menuItem;
113-
114-
if (type === 'item') {
115-
return (
116-
<a key={`${type}-${content}`} className="nav-link" href={href}>{content}</a>
117-
);
118-
}
119-
120-
return (
121-
<Menu key={`${type}-${content}`} tag="div" className="nav-item" respondToPointerEvents>
122-
<MenuTrigger tag="a" className="nav-link d-inline-flex align-items-center" href={href}>
123-
{content} <CaretIcon role="img" aria-hidden focusable="false" />
124-
</MenuTrigger>
125-
<MenuContent className="pin-left pin-right shadow py-2">
126-
{submenuContent}
127-
</MenuContent>
128-
</Menu>
129-
);
130-
});
131-
}
132-
133106
renderLoggedOutItems() {
134107
const { loggedOutItems } = this.props;
135108

‎src/Header.jsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ ensureConfig([
2727
subscribe(APP_CONFIG_INITIALIZED, () => {
2828
mergeConfig({
2929
AUTHN_MINIMAL_HEADER: !!process.env.AUTHN_MINIMAL_HEADER,
30-
ENABLE_HEADER_WITHOUT_USERNAME: !!process.env.ENABLE_HEADER_WITHOUT_USERNAME,
30+
HIDE_USERNAME_IN_HEADER: !!process.env.HIDE_USERNAME_IN_HEADER,
3131
}, 'Header additional config');
3232
});
3333

‎src/Header.messages.jsx

+4-9
Original file line numberDiff line numberDiff line change
@@ -76,15 +76,10 @@ const messages = defineMessages({
7676
defaultMessage: 'Account Menu',
7777
description: 'The aria label for the account menu trigger',
7878
},
79-
'header.label.account.menu.with.username': {
80-
id: 'header.label.account.menu.with.username',
81-
defaultMessage: 'Account menu for {username}',
82-
description: 'The aria label for the account menu trigger when the username is displayed in it',
83-
},
84-
'header.label.account.menu.without.username': {
85-
id: 'header.label.account.without.username',
86-
defaultMessage: 'Account menu for {name}',
87-
description: 'The aria label for the account menu trigger when ENABLE_HEADER_WITHOUT_USERNAME is enabled',
79+
'header.label.account.menu.using.name.or.username': {
80+
id: 'header.label.account.menu.using.name.or.username',
81+
defaultMessage: 'Account menu for {usernameOrName}',
82+
description: 'The aria label for the account menu trigger when the username is displayed or hide in it',
8883
},
8984
'header.label.main.nav': {
9085
id: 'header.label.main.nav',

‎src/Header.test.jsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ describe('<Header />', () => {
7676
LOGIN_URL: process.env.LOGIN_URL,
7777
LOGOUT_URL: process.env.LOGOUT_URL,
7878
LOGO_URL: process.env.LOGO_URL,
79-
ENABLE_HEADER_WITHOUT_USERNAME: true,
79+
HIDE_USERNAME_IN_HEADER: true,
8080
},
8181
};
8282
const component = <HeaderComponent width={{ width: 1280 }} contextValue={contextValue} />;

‎src/__snapshots__/Header.test.jsx.snap

-2
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,6 @@ exports[`<Header /> renders correctly for authenticated desktop with username 1`
281281
</svg>
282282
</span>
283283
edX
284-
285284
<svg
286285
aria-hidden={true}
287286
focusable="false"
@@ -389,7 +388,6 @@ exports[`<Header /> renders correctly for authenticated desktop without username
389388
/>
390389
</svg>
391390
</span>
392-
393391
<svg
394392
aria-hidden={true}
395393
focusable="false"

‎src/learning-header/AuthenticatedUserDropdown.jsx

+9-7
Original file line numberDiff line numberDiff line change
@@ -4,37 +4,39 @@ import PropTypes from 'prop-types';
44
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
55
import { faUserCircle } from '@fortawesome/free-solid-svg-icons';
66
import { getConfig } from '@edx/frontend-platform';
7-
import { Avatar } from '@openedx/paragon';
87
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
9-
import { Dropdown } from '@openedx/paragon';
8+
import { Avatar, Dropdown } from '@openedx/paragon';
109

1110
import messages from './messages';
1211

1312
const AuthenticatedUserDropdown = ({
14-
intl, username, avatar, name
13+
intl, username, avatar, name,
1514
}) => {
1615
const dashboardMenuItem = (
1716
<Dropdown.Item href={`${getConfig().LMS_BASE_URL}/dashboard`}>
1817
{intl.formatMessage(messages.dashboard)}
1918
</Dropdown.Item>
2019
);
2120

22-
const showDropdownToggle = (
21+
// show avatar instead username if HIDE_USERNAME_IN_HEADER flag is enabled
22+
const dropdownToggle = (
2323
<Dropdown.Toggle variant="outline-primary">
2424
<FontAwesomeIcon icon={faUserCircle} className="d-md-none" size="lg" />
25-
{!getConfig().ENABLE_HEADER_WITHOUT_USERNAME ? (
25+
{getConfig().HIDE_USERNAME_IN_HEADER ? (
26+
<Avatar size="sm" src={avatar} alt={name} className="mr-2" />
27+
) : (
2628
<span data-hj-suppress className="d-none d-md-inline" data-testid="username">
2729
{username}
2830
</span>
29-
) : <Avatar size="sm" src={avatar} alt={name} className="mr-2" />}
31+
)}
3032
</Dropdown.Toggle>
3133
);
3234

3335
return (
3436
<>
3537
<a className="text-gray-700" href={`${getConfig().SUPPORT_URL}`}>{intl.formatMessage(messages.help)}</a>
3638
<Dropdown className="user-dropdown ml-3">
37-
{showDropdownToggle}
39+
{dropdownToggle}
3840
<Dropdown.Menu className="dropdown-menu-right">
3941
{dashboardMenuItem}
4042
<Dropdown.Item href={`${getConfig().ACCOUNT_PROFILE_URL}/u/${username}`}>

‎src/learning-header/LearningHeader.test.jsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ describe('Header', () => {
2020
const config = getConfig();
2121
mergeConfig({
2222
...config,
23-
ENABLE_HEADER_WITHOUT_USERNAME: true,
23+
HIDE_USERNAME_IN_HEADER: true,
2424
});
2525
const { queryByText } = render(<Header />);
2626
const userName = queryByText(config.authenticatedUser.username);

‎src/studio-header/StudioHeader.test.jsx

+8-3
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
/* eslint-disable react/prop-types */
22
import React, { useMemo } from 'react';
3+
import { getConfig, mergeConfig } from '@edx/frontend-platform';
34
import {
45
render,
56
fireEvent,
@@ -128,9 +129,13 @@ describe('Header', () => {
128129
expect(avatarIcon).toBeVisible();
129130
});
130131

131-
it.only('user menu should not contain username', async () => {
132-
const newProps = { ...props, ENABLE_HEADER_WITHOUT_USERNAME: true };
133-
const { container } = render(<RootWrapper {...newProps} />);
132+
it('user menu should not contain username', async () => {
133+
const config = getConfig();
134+
mergeConfig({
135+
...config,
136+
HIDE_USERNAME_IN_HEADER: true,
137+
});
138+
const { container } = render(<RootWrapper {...props} />);
134139
const userMenue = container.querySelector('#user-dropdown-menu');
135140
expect(userMenue.textContent).toContain('');
136141
});

‎src/studio-header/UserMenu.jsx

+3-3
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,8 @@ const UserMenu = ({
1919
// injected
2020
intl,
2121
}) => {
22-
const showUsername = !getConfig().ENABLE_HEADER_WITHOUT_USERNAME;
23-
const avatarAlt = showUsername ? username : name;
22+
const hideUsername = getConfig().HIDE_USERNAME_IN_HEADER;
23+
const avatarAlt = hideUsername ? name : username;
2424
const avatar = authenticatedUserAvatar ? (
2525
<img
2626
className="d-block w-100 h-100"
@@ -36,7 +36,7 @@ const UserMenu = ({
3636
data-testid="avatar-icon"
3737
/>
3838
);
39-
const title = isMobile ? avatar : <>{avatar}{showUsername && username}</>;
39+
const title = (isMobile || hideUsername) ? avatar : <>{avatar}{username}</>;
4040

4141
return (
4242
<NavDropdownMenu

0 commit comments

Comments
 (0)