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: add user email and name in user dropdown menu #558

Merged
merged 1 commit into from Mar 20, 2024

Conversation

mubbsharanwar
Copy link
Contributor

@mubbsharanwar mubbsharanwar commented Feb 29, 2024

We are removing the username from the registration form that's why we are removing the username from the user-facing views and adding the user email and full name in the user menu dropdown.

Screenshort

Component Before Desktop View After Desktop View Before Mobile View After Mobile View
Header Screenshot 2024-03-07 at 10 44 40 PM Screenshot 2024-03-05 at 3 51 13 PM Screenshot 2024-03-05 at 3 51 48 PM Screenshot 2024-03-12 at 12 45 08 PM
Learning Header Screenshot 2024-03-07 at 10 41 20 PM Screenshot 2024-03-05 at 4 02 54 PM Screenshot 2024-03-07 at 10 42 04 PM Screenshot 2024-03-06 at 3 36 50 PM
Studio Header Screenshot 2024-03-07 at 10 42 55 PM Screenshot 2024-03-05 at 3 55 51 PM Screenshot 2024-03-07 at 10 43 36 PM Screenshot 2024-03-05 at 3 57 06 PM

VAN-1851

@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 3 times, most recently from fca4ef8 to dadee8e Compare February 29, 2024 10:28
@codecov-commenter
Copy link

codecov-commenter commented Feb 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.32%. Comparing base (2bba7f5) to head (be9c582).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #558   +/-   ##
=======================================
  Coverage   72.32%   72.32%           
=======================================
  Files          51       52    +1     
  Lines         813      813           
  Branches      163      158    -5     
=======================================
  Hits          588      588           
  Misses        215      215           
  Partials       10       10           

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

@mubbsharanwar mubbsharanwar marked this pull request as ready for review February 29, 2024 12:22
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 3 times, most recently from 2e55de9 to 4202bcc Compare March 1, 2024 05:24
src/Header.jsx Outdated Show resolved Hide resolved
src/Header.jsx Outdated Show resolved Hide resolved
src/Header.jsx Outdated Show resolved Hide resolved
src/index.scss Outdated Show resolved Hide resolved
src/learning-header/AuthenticatedUserDropdown.jsx Outdated Show resolved Hide resolved
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from 12ffa7c to a32a5ff Compare March 1, 2024 10:51
src/common/UserMenuItem.jsx Outdated Show resolved Hide resolved
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.

Can you please add before and after screenshots?

example/index.js Outdated Show resolved Hide resolved
src/common/UserMenuItem.jsx Outdated Show resolved Hide resolved
src/Header.jsx Outdated Show resolved Hide resolved
src/hook.jsx Outdated Show resolved Hide resolved
src/index.scss Outdated Show resolved Hide resolved
@syedsajjadkazmii syedsajjadkazmii self-requested a review March 6, 2024 11:08
@syedsajjadkazmii syedsajjadkazmii dismissed their stale review March 6, 2024 11:09

Will review again

@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from a2258c9 to c403993 Compare March 6, 2024 11:20
example/index.js Outdated Show resolved Hide resolved
src/Header.jsx Outdated Show resolved Hide resolved
src/index.scss Outdated Show resolved Hide resolved
src/studio-header/UserMenu.jsx Show resolved Hide resolved
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from 59b333f to 2869cfc Compare March 11, 2024 11:57
src/Header.jsx Outdated Show resolved Hide resolved
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from 3a746dd to 82783e2 Compare March 12, 2024 08:50
@@ -78,7 +78,7 @@ const messages = defineMessages({
},
'header.label.account.menu.for': {
id: 'header.label.account.menu.for',
defaultMessage: 'Account menu for {username}',
defaultMessage: 'Account menu for {name}',
description: 'The aria label for the account menu trigger when the username is displayed in it',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: 'The aria label for the account menu trigger when the username is displayed in it',
description: 'The aria label for the account menu trigger',

@@ -37,7 +37,7 @@ exports[`<Header /> minimal renders correctly for authenticated users when minim
alt=""
aria-expanded={false}
aria-haspopup={true}
aria-label="Account menu for edX"
aria-label="Account menu for "
Copy link
Member

Choose a reason for hiding this comment

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

Why is the name missing in the aria-label? Probably the test needs to be fixed.

@@ -293,7 +291,7 @@ exports[`<Header /> renders correctly for authenticated users on mobile 1`] = `
type="button"
>
<img
alt="edX"
alt={null}
Copy link
Member

Choose a reason for hiding this comment

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

This should not be null

src/common/UserMenuItem.jsx Outdated Show resolved Hide resolved
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.

Dont we want to make changes in src/MobileHeader.jsx?

src/studio-header/utils.js Outdated Show resolved Hide resolved
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from 3964710 to 38b2148 Compare March 15, 2024 11:04
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 3 times, most recently from 74798c0 to 372ba7a Compare March 18, 2024 11:53
@mubbsharanwar mubbsharanwar changed the title feat: add user email in header feat: add user email and name in user dropdown menu Mar 18, 2024
@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 2 times, most recently from e75bac6 to 5b1d37c Compare March 18, 2024 14:24
@mubbsharanwar
Copy link
Contributor Author

Dont we want to make changes in src/MobileHeader.jsx?

We added

Screenshot 2024-03-18 at 7 25 31 PM

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.

Some minor comments. Thanks

src/Header.jsx Show resolved Hide resolved
src/MobileHeader.jsx Outdated Show resolved Hide resolved
src/common/style.scss Outdated Show resolved Hide resolved
src/studio-header/NavDropdownMenu.jsx Show resolved Hide resolved
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.

LGTM.

@mubbsharanwar mubbsharanwar force-pushed the manwar/VAN-1851 branch 3 times, most recently from a379834 to b966f33 Compare March 19, 2024 06:19
@mubbsharanwar mubbsharanwar merged commit 2e76a03 into master Mar 20, 2024
6 checks passed
@mubbsharanwar mubbsharanwar deleted the manwar/VAN-1851 branch March 20, 2024 06:38
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.

None yet

5 participants