Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove username from header #551

Merged
merged 1 commit into from
Feb 19, 2024
Merged

feat: remove username from header #551

merged 1 commit into from
Feb 19, 2024

Conversation

mubbsharanwar
Copy link
Contributor

@mubbsharanwar mubbsharanwar commented Feb 13, 2024

In the spirit of making registration experience quick and easy (more details on MVP scope doc), 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
Set the flag HIDE_USERNAME_FROM_HEADER to hide the username from the header and replace the username with the name for the screen reader text when the flag is enabled.
The flag will be removed when this PR would be merged

Screenshots

Component Before After
Header Screenshot 2024-02-13 at 1 31 38 PM Screenshot 2024-02-13 at 1 32 45 PM
Studio Header Screenshot 2024-02-13 at 1 36 17 PM Screenshot 2024-02-13 at 1 35 17 PM
Learning Header Screenshot 2024-02-13 at 1 38 22 PM Screenshot 2024-02-13 at 1 39 38 PM

Ticket
VAN-1804

@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from 3a42e81 to 2399197 Compare February 13, 2024 08:59
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (011b25d) 71.94% compared to head (11981e5) 72.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #551      +/-   ##
==========================================
+ Coverage   71.94%   72.32%   +0.37%     
==========================================
  Files          51       51              
  Lines         802      813      +11     
  Branches      156      163       +7     
==========================================
+ Hits          577      588      +11     
  Misses        215      215              
  Partials       10       10              

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

Copy link
Contributor

@syedsajjadkazmii syedsajjadkazmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work on the MFEs that are on the openedx organization? like frontend-app-learner-dashboard?

src/learning-header/LearningHeader.test.jsx Outdated Show resolved Hide resolved
src/studio-header/UserMenu.jsx Outdated Show resolved Hide resolved
src/studio-header/UserMenu.jsx Show resolved Hide resolved
@mubbsharanwar
Copy link
Contributor Author

Would this work on the MFEs that are on the openedx organization? like frontend-app-learner-dashboard?

We set the npm alias to indicate from where the header component would be installed. For now, aliase is not added for frontend-app-learner-dashboard but we can add it to install frontend-component-header-edx.
Reference to add npm alias :https://github.com/edx/edx-internal/blob/master/frontends/frontend-app-learner-record/prod_config.yml#L6

@syedsajjadkazmii
Copy link
Contributor

Would this work on the MFEs that are on the openedx organization? like frontend-app-learner-dashboard?

We set the npm alias to indicate from where the header component would be installed. For now, aliase is not added for frontend-app-learner-dashboard but we can add it to install frontend-component-header-edx. Reference to add npm alias :https://github.com/edx/edx-internal/blob/master/frontends/frontend-app-learner-record/prod_config.yml#L6

frontend-app-learner-dashboard was just an example, we would need to do this for all MFEs residing in openedX repo.

@mubbsharanwar
Copy link
Contributor Author

mubbsharanwar commented Feb 14, 2024

Would this work on the MFEs that are on the openedx organization? like frontend-app-learner-dashboard?

We set the npm alias to indicate from where the header component would be installed. For now, aliase is not added for frontend-app-learner-dashboard but we can add it to install frontend-component-header-edx. Reference to add npm alias :https://github.com/edx/edx-internal/blob/master/frontends/frontend-app-learner-record/prod_config.yml#L6

frontend-app-learner-dashboard was just an example, we would need to do this for all MFEs residing in openedX repo.

Yes we can install in openedX repos but we need to add alias in edx-internal, for most of MFEs alias is added, you can check the detail here.
And I already shared the reference with you how to add alias.

Copy link
Contributor

@syedsajjadkazmii syedsajjadkazmii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou

src/Header.jsx Outdated
@@ -32,6 +32,9 @@ subscribe(APP_CONFIG_INITIALIZED, () => {
MINIMAL_HEADER: !!process.env.MINIMAL_HEADER,
ENTERPRISE_LEARNER_PORTAL_HOSTNAME: process.env.ENTERPRISE_LEARNER_PORTAL_HOSTNAME,
AUTHN_MINIMAL_HEADER: !!process.env.AUTHN_MINIMAL_HEADER,
// this flag is introduced to unblock for new release.
// the code behind flag would be removed in follow up PR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code would be removed or the flag would be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flag. comment updated

Comment on lines 26 to 27
const userName = queryByTestId('username');
expect(userName).not.toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const userName = queryByTestId('username');
expect(userName).not.toBeInTheDocument();
const username = queryByTestId('username');
expect(username).not.toBeInTheDocument();

Comment on lines 140 to 141
const userMenue = container.querySelector('#user-dropdown-menu');
expect(userMenue.textContent).toContain('');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const userMenue = container.querySelector('#user-dropdown-menu');
expect(userMenue.textContent).toContain('');
const userMenu = container.querySelector('#user-dropdown-menu');
expect(userMenu.textContent).toContain('');

data-hj-suppress
>
{username}
{!hideUsername && username}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be

Suggested change
{!hideUsername && username}
{usernameOrName}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't want to show username or name when HIDE_USERNAME_FROM_HEADER is enabled.

};

AuthenticatedUserDropdown.defaultProps = {
enterpriseLearnerPortalLink: '',
name: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be string

Suggested change
name: null,
name: '',

@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1804 branch 2 times, most recently from beef0db to 4c22911 Compare February 19, 2024 07:50
Remove username from header and add name for screen reader

VAN-1804
@mubbsharanwar mubbsharanwar merged commit 1e9af3c into master Feb 19, 2024
6 checks passed
@mubbsharanwar mubbsharanwar deleted the manwar/VAN-1804 branch February 19, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants