feat: set username behind flag by mubbsharanwar · Pull Request #465 · openedx/frontend-component-header

mubbsharanwar · 2024-01-31T14:09:52Z

In the spirit of making registration experience quick and easy (more details on the proposal page in the wiki, we want to be able to remove username from the registration form. This creates the need to remove it everywhere on the UX too. As part of this initiative, we will be removing username from the headers and making the dropdown consistent with edx.org site.

Changes
Added another variation where header is rendered without the username. This variation is kept behind the HIDE_USERNAME_FROM_HEADER environment variable. By default, this feature is turned off.

Screenshots

Component	Feature flag disabled	Feature flag enabled
Header
Studio Header
Learning Header

Ticket
VAN-1804

zainab-amir · 2024-02-07T06:45:27Z

    });

+    it.only('user menu should not contain username', async () => {
+      const newProps = { ...props, ENABLE_HEADER_WITHOUT_USERNAME: true };


Why are we sending the environment variable as a prop?

Mistakenly added it here, We are enabling this flag using mergeConfig.

mubbsharanwar · 2024-02-07T09:00:06Z

@zainab-amir Addressed your comments.

codecov · 2024-02-09T01:46:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3b2a2bf) 66.96% compared to head (fcaa2b6) 68.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #465      +/-   ##
==========================================
+ Coverage   66.96%   68.09%   +1.12%     
==========================================
  Files          25       25              
  Lines         339      351      +12     
  Branches       78       85       +7     
==========================================
+ Hits          227      239      +12     
  Misses        110      110              
  Partials        2        2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

syedsajjadkazmii

Minor comments. Thankyou

zainab-amir · 2024-02-09T12:14:21Z

    </Dropdown.Item>
  );

+  // show avatar instead username if HIDE_USERNAME_FROM_HEADER flag is enabled


Suggested change

// show avatar instead username if HIDE_USERNAME_FROM_HEADER flag is enabled

// show avatar instead of username if `HIDE_USERNAME_FROM_HEADER flag` is enabled

username would be show in header bt enabling ENABLE_HEADER_WITHOUT_USERNAME flag. VAN-1804 fix: address reviewer comments

adamstankiewicz · 2024-02-12T17:08:25Z

@mubbsharanwar Given this is making a change to the shared Open edX component header, should we seek reviewers/approval in the community Frontend Working Group or other stakeholders from the Open edX community?

Otherwise, if these changes are specific to a 2U/edX.org decision, should these changes be made in the frontend-component-header-edx repo instead? Generally, the Open edX version of the shared header component is replaced by @edx/frontend-component-header-edx via an NPM alias during build+deploy in 2U/edX's GoCD pipeline for MFEs deployed to *.edx.org.

zainab-amir · 2024-02-13T07:10:09Z

@adamstankiewicz thank you for sharing the information on the override for frontend-component-header. We will be making these changes to @edx/frontend-component-header-edx (we won't even need to keep this change behind a flag!)

brian-smith-tcril

Overall this looks very cleanly implemented! I left a couple of comments with questions and I'm looking forward to hearing what others think about those.

brian-smith-tcril · 2024-02-13T20:39:50Z

 * ``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
+* ``HIDE_USERNAME_FROM_HEADER`` - A boolean flag which hides the username from the header


General question (hoping to hear what others think): Do we prefer this as HIDE or as SHOW? I could be persuaded either way.

Come to think of it, I could even see this being non-boolean: something like NAME_IN_MENU_BUTTON with the options being username, fullName, none

brian-smith-tcril · 2024-02-13T20:42:40Z

        authenticatedUser: {
          userId: '123abc',
          username: 'testuser',
+          name: 'test user',


Another general question: Would we prefer this as fullName? With this test data I'm assuming we'd expect both first and last name in here. If that's the case I feel fullName is more descriptive.

The authenticated data sent in JWT has "name" not "fullName"

brian-smith-tcril · 2024-02-13T20:58:08Z

        <MenuTrigger
          tag="button"
-          aria-label={intl.formatMessage(messages['header.label.account.menu.for'], { username })}
+          aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })}


Another open question: Do we want to use a new message key here?

An alternative solution that comes to mind is

Suggested change

aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })}

aria-label={intl.formatMessage(messages['header.label.account.menu.for'], { username: usernameOrName })}

brian-smith-tcril · 2024-02-13T21:12:52Z

name should be added to the authenticatedUser in the context for the StudioHeader too

brian-smith-tcril · 2024-02-13T21:15:01Z

+          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" />


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?

brian-smith-tcril · 2024-02-13T21:24:48Z

+    const hideUsername = getConfig().HIDE_USERNAME_FROM_HEADER;
+    const usernameOrName = hideUsername ? name : username;


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.

We will be auto-generating username and showing that in alt text doesn't make sense. I can see how HIDE_USERNAME_FROM_HEADER doesn't provide more context around this.

Maybe we can go with your suggestion of NAME_IN_MENU_BUTTON. Which tells us whether we want to use username or fullName in the header and use the same in screen reader text too.

jmakowski1123 · 2024-02-14T16:57:35Z

Thanks for flagging this for product review. The linked "MVP scope doc" which I presume has additional product context to help make a decision is not available. Could this doc please be added to the Open edX product wiki? I created a space where you can copy/paste the doc: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4060938253/Proposal+-+Set+username+behind+flag+across+MFE+headers

arbrandes

Aside from the whole product discussion, as I've mentioned below (and elsewhere), I don't think it makes architectural sense for the logic to select between username and name to reside in DesktopHeader.

Also, given the scope of the proposal, I believe the decision of what to show the user what their identifier is (whether username, full name, email, or blank) should be made in a place closer to the data source (like in edx-platform), and just passed along as a string in the REST API and down props until it gets to the header.

arbrandes · 2024-04-19T13:52:11Z

+      intl,
      userMenu,
      avatar,
+      name,


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 and name. It should just use a single prop, and the business logic of what to actually put there should be up to the user of the component.

zainab-amir · 2024-04-24T15:38:08Z

This PR is not needed anymore. For edX we have solved this by npm aliasing the header with edX specific header.

mubbsharanwar force-pushed the manwar/VAN-1804 branch 7 times, most recently from ccc6023 to e0b060d Compare February 6, 2024 03:08

mubbsharanwar marked this pull request as ready for review February 6, 2024 03:08

mubbsharanwar force-pushed the manwar/VAN-1804 branch 3 times, most recently from 61df27f to cf0f69a Compare February 6, 2024 03:44

mubbsharanwar changed the title ~~feat: remove username~~ feat: set username behind flag Feb 6, 2024

mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from 66a0c4c to 4cc31ca Compare February 7, 2024 03:00

zainab-amir reviewed Feb 7, 2024

View reviewed changes

mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from 2a34074 to c5935e5 Compare February 7, 2024 08:56

zainab-amir reviewed Feb 7, 2024

View reviewed changes

Comment thread src/learning-header/AuthenticatedUserDropdown.jsx Outdated

Comment thread src/Header.messages.jsx Outdated

Comment thread src/DesktopHeader.jsx Outdated

syedsajjadkazmii reviewed Feb 7, 2024

View reviewed changes

Comment thread README.rst Outdated

Comment thread src/DesktopHeader.jsx Outdated

Comment thread src/learning-header/AuthenticatedUserDropdown.jsx Outdated

Comment thread src/learning-header/AuthenticatedUserDropdown.jsx Outdated

mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from dd978ec to dbda420 Compare February 9, 2024 01:42

syedsajjadkazmii reviewed Feb 9, 2024

View reviewed changes

Comment thread README.rst Outdated

Comment thread src/DesktopHeader.jsx Outdated

Comment thread src/learning-header/AuthenticatedUserDropdown.jsx Outdated

syedsajjadkazmii reviewed Feb 9, 2024

View reviewed changes

mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from ef31ead to 7fa9371 Compare February 9, 2024 11:38

syedsajjadkazmii approved these changes Feb 9, 2024

View reviewed changes

zainab-amir approved these changes Feb 9, 2024

View reviewed changes

feat: set username behind flag

fcaa2b6

username would be show in header bt enabling ENABLE_HEADER_WITHOUT_USERNAME flag. VAN-1804 fix: address reviewer comments

mubbsharanwar force-pushed the manwar/VAN-1804 branch from 7fa9371 to fcaa2b6 Compare February 12, 2024 04:58

zainab-amir closed this Feb 13, 2024

zainab-amir deleted the manwar/VAN-1804 branch February 13, 2024 07:10

zainab-amir restored the manwar/VAN-1804 branch February 13, 2024 14:13

zainab-amir reopened this Feb 13, 2024

brian-smith-tcril reviewed Feb 13, 2024

View reviewed changes

mubbsharanwar mentioned this pull request Feb 16, 2024

feat: remove username from header edx/frontend-component-header-edx#551

Merged

This was referenced Apr 18, 2024

feat: display profile name in navbar if available openedx/frontend-app-learner-dashboard#313

Closed

feat: display profile name in navbar if available #483

Closed

arbrandes requested changes Apr 19, 2024

View reviewed changes

zainab-amir closed this Apr 24, 2024

zainab-amir deleted the manwar/VAN-1804 branch April 24, 2024 15:38

	// show avatar instead username if HIDE_USERNAME_FROM_HEADER flag is enabled
	// show avatar instead of username if `HIDE_USERNAME_FROM_HEADER flag` is enabled

	aria-label={intl.formatMessage(messages['header.label.account.menu.using.name.or.username'], { usernameOrName })}
	aria-label={intl.formatMessage(messages['header.label.account.menu.for'], { username: usernameOrName })}

		const hideUsername = getConfig().HIDE_USERNAME_FROM_HEADER;
		const usernameOrName = hideUsername ? name : username;

Uh oh!

Conversation

mubbsharanwar commented Jan 31, 2024 • edited by arbrandes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mubbsharanwar commented Feb 7, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

syedsajjadkazmii left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamstankiewicz commented Feb 12, 2024

Uh oh!

zainab-amir commented Feb 13, 2024

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brian-smith-tcril Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zainab-amir Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zainab-amir Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmakowski1123 commented Feb 14, 2024

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zainab-amir commented Apr 24, 2024

Uh oh!

Reviewers

Assignees

mubbsharanwar commented Jan 31, 2024 •

edited by arbrandes

Loading

codecov Bot commented Feb 9, 2024 •

edited

Loading

brian-smith-tcril Feb 13, 2024 •

edited

Loading

zainab-amir Feb 14, 2024 •

edited

Loading

zainab-amir Feb 14, 2024 •

edited

Loading