change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383

bnussman-akamai · 2024-04-16T17:26:02Z

Description 📝

Cleans up and fixes many inconsistencies with the Top Menu 🧹

Fixes

1. Fixes bug where opening the Notification/Event Menu would scroll to top in Safari browsers 🔧

Before	After
Screen.Recording.2024-04-16.at.1.18.03.PM.mov	Screen.Recording.2024-04-16.at.1.18.21.PM.mov

2. Fixes inconsistent ripple effects

Before	After
Screen.Recording.2024-04-16.at.1.11.08.PM.mov	Screen.Recording.2024-04-16.at.1.13.24.PM.mov

3. Makes Notification/Event Menu work and look like the `UserMenu` and the `AddNewMenu` 🎨

Before	After
Screen.Recording.2024-04-16.at.1.15.16.PM.mov	Screen.Recording.2024-04-16.at.1.15.26.PM.mov

Other Changes 🔁

Makes "Help and Support" and actual Link, so it should be more accessible
Makes "Community" an actual Link, so it should be more accessible

How to test 🧪

Test the Top Menu
Verify accessibility does not get worse

As an Author I have considered 🤔

bnussman-akamai · 2024-04-17T15:16:01Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenu.tsx

      >
-        <ClickAwayListener onClickAway={handleClose}>
-          <MenuList


As far as I can tell, this Menu stuff isn't even doing anything meaningful. For it to work properly, the direct children probably need to be Menu Items.

bnussman-akamai · 2024-04-17T15:16:41Z

packages/manager/src/features/TopMenu/TopMenu.tsx

@@ -64,7 +64,7 @@ export const TopMenu = React.memo((props: TopMenuProps) => {
          variant="dense"
        >
          <Hidden mdDown>
-            <TopMenuIcon key={navHoverText} title={navHoverText}>
+            <TopMenuTooltip title={navHoverText}>


I don't think key is needed here. Please correct me if I'm wrong

bnussman-akamai · 2024-04-17T15:17:47Z

packages/manager/src/features/TopMenu/TopMenuTooltip.tsx

I just renamed TopMenuIcon.tsx to TopMenuTooltip.tsx because TopMenuIcon didn't make sense

bnussman-akamai · 2024-04-17T15:22:32Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenu.tsx

+          )}
+        </IconButton>
+      </TopMenuTooltip>
+      <Popover


Now using a Popover instead of a Popper to match the behavior of the AddNewMenu and the UserMenu. This fixes a lot of styling and adds a nice animation to match the other menus.

Before This PR

Screen.Recording.2024-04-17.at.11.19.39.AM.mov

Screen.Recording.2024-04-17.at.11.21.53.AM.mov

Notice how the Popper background does not stay when scroll bounces and how there is no open/close animation Notice how the background of the Popover is constant when scroll bounces and there is a nice open/close animation

bnussman-akamai · 2024-04-17T15:23:16Z

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenu.tsx

-            padding: 0,
-          }}
+      <TopMenuTooltip title="Notifications">
+        <IconButton


An IconButton is much more fitting than a Button

bnussman-akamai · 2024-04-17T15:24:19Z

packages/manager/src/features/TopMenu/Help.tsx

-      </StyledLinkButton>
-    </TopMenuIcon>
+    <TopMenuTooltip title="Help & Support">
+      <Link to="/support">


Using an actual Link instead of using JS to go to /support. This is probably good for accessibility and probably just better practice?

bnussman-akamai · 2024-04-17T15:24:48Z

packages/manager/src/features/TopMenu/Help.tsx

-        role="link"
-      >
-        <StyledTopMenuIconWrapper>
-          <HelpSVGIcon status="help" />


I don't think status="help" does anything meaningful. Let me know if I'm mistaken

This status="help" prop and the other one you removed from the Community component status="community" appear to be remaining bits from previous implementations of the tooltip icons. I agree that these don't belong in the codebase any longer. Good catch.

github-actions · 2024-04-17T16:42:59Z

Coverage Report: ❌
Base Coverage: 81.85%
Current Coverage: 81.77%

carrillo-erik · 2024-04-17T20:56:21Z

I ran the lighthouse tool against your PR and the develop branch. Although the tool is not perfect it does help point out some useful information. In this case Accessibility dropped from 92 to 87. It appears to be complaining about the IconButton not having any text for screen readers.

Develop

This PR

The good news is that the workaround seems straightforward based on the recommendations. I tried aria-label="Help", I suppose the same can be done with accessibleAriaLabel. The Accessibility score remained the same, however, the message went away.

packages/manager/src/features/TopMenu/Community.tsx

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenu.tsx

…ed of a button

abailly-akamai

Thanks for addressing the fixes. looks good and consistent. This is a nice cleanup!

Semantic HTML & accessibility ✅
Behavior & no regressions ✅

I noticed that we don't have a rel="noopener noreferrer" for the cloud community link but I don't think we even need those anymore as it seems to have been set as the default behavior in most browsers. Not to mentioned, the risk of browser hijacking by our own web properties seems fairly low 😄

Fixes a few console errors caused by change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383 Sorry!! 🤦 Allows our Link component to accept a ref (had to do this to fix Tooltip ref error) Co-authored-by: Banks Nussman <banks@nussman.us>

bnussman added 2 commits April 16, 2024 12:43

clean up top menu

4fbce85

fix bell styles

9abb09f

bnussman-akamai self-assigned this Apr 16, 2024

fix storybook build

39ccb9c

bnussman-akamai added Enhancement UX/UI Changes for UI/UX to review labels Apr 16, 2024

bnussman added 4 commits April 16, 2024 15:20

Added changeset: Top Menu clean up and refactor

e0dc5ba

make small screen sizes match prod

52362b0

remove unnessesary keys

c2d2028

clean up

a67c456

bnussman-akamai marked this pull request as ready for review April 17, 2024 15:14

bnussman-akamai requested a review from a team as a code owner April 17, 2024 15:14

bnussman-akamai requested review from hana-akamai and abailly-akamai and removed request for a team April 17, 2024 15:14

bnussman-akamai commented Apr 17, 2024

View reviewed changes

bnussman-akamai requested review from mjac0bs and carrillo-erik April 17, 2024 15:33

fix e2e test

862e8db

bnussman-akamai requested a review from a team as a code owner April 17, 2024 16:17

bnussman-akamai requested review from cliu-akamai and removed request for a team April 17, 2024 16:17

carrillo-erik approved these changes Apr 17, 2024

View reviewed changes

abailly-akamai reviewed Apr 18, 2024

View reviewed changes

packages/manager/src/features/TopMenu/Community.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/TopMenu/NotificationMenu/NotificationMenu.tsx Outdated Show resolved Hide resolved

fix lighthouse score - improve accessability by rendering a link inst…

b4f82ab

…ed of a button

bnussman-akamai requested a review from abailly-akamai April 18, 2024 14:26

bnussman added 2 commits April 18, 2024 10:41

improvements and feedback @abailly-akamai

a3a2cee

revert notifications aria-label change

6f442e8

abailly-akamai approved these changes Apr 19, 2024

View reviewed changes

bnussman-akamai merged commit 8b2ec94 into linode:develop Apr 19, 2024
18 checks passed

bnussman-akamai mentioned this pull request Apr 23, 2024

fix: TopMenu related console errors #10398

Merged

12 tasks

This was referenced Apr 25, 2024

Release v.118.0 - release → staging #10409

Closed

Release v1.118.0 - release → staging #10410

Merged

Release v1.118.0 - staging → master #10416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383

change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383

bnussman-akamai commented Apr 16, 2024 •

edited

Loading

bnussman-akamai Apr 17, 2024

bnussman-akamai Apr 17, 2024

bnussman-akamai Apr 17, 2024 •

edited

Loading

bnussman-akamai Apr 17, 2024

bnussman-akamai Apr 17, 2024

bnussman-akamai Apr 17, 2024

bnussman-akamai Apr 17, 2024

carrillo-erik Apr 17, 2024

github-actions bot commented Apr 17, 2024 •

edited

Loading

carrillo-erik commented Apr 17, 2024

abailly-akamai left a comment

Before	This PR
Screen.Recording.2024-04-17.at.11.19.39.AM.mov	Screen.Recording.2024-04-17.at.11.21.53.AM.mov
Notice how the Popper background does not stay when scroll bounces and how there is no open/close animation	Notice how the background of the Popover is constant when scroll bounces and there is a nice open/close animation

change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383

change: [M3-7994] - Clean up Top Menu and Fix Inconsistencies #10383

Conversation

bnussman-akamai commented Apr 16, 2024 • edited Loading

Description 📝

Fixes

1. Fixes bug where opening the Notification/Event Menu would scroll to top in Safari browsers 🔧

2. Fixes inconsistent ripple effects

3. Makes Notification/Event Menu work and look like the UserMenu and the AddNewMenu 🎨

Other Changes 🔁

How to test 🧪

As an Author I have considered 🤔

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

bnussman-akamai Apr 17, 2024

Choose a reason for hiding this comment

carrillo-erik Apr 17, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 17, 2024 • edited Loading

carrillo-erik commented Apr 17, 2024

Develop

This PR

abailly-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Apr 16, 2024 •

edited

Loading

3. Makes Notification/Event Menu work and look like the `UserMenu` and the `AddNewMenu` 🎨

bnussman-akamai Apr 17, 2024 •

edited

Loading

github-actions bot commented Apr 17, 2024 •

edited

Loading