Shadcn migration - mobile menu #13449

pettinarip · 2024-07-23T08:47:55Z

Description

Mobile menu implementation using Shadcn.

Related Issue

Depends on #13411 and #13419

src/components/Nav/Mobile/LvlAccordion.tsx

src/hooks/useDisclosure.ts

src/components/Nav/Mobile/index.tsx

netlify · 2024-07-30T13:57:31Z

✅ Deploy Preview for ethereumorg ready!

Name	Link
🔨 Latest commit	`a0a4760`
🔍 Latest deploy log	https://app.netlify.com/sites/ethereumorg/deploys/66b2943ac5289000084152e2
😎 Deploy Preview	https://deploy-preview-13449--ethereumorg.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	7 paths audited Performance: 48 (🔴 down 4 from production) Accessibility: 93 (no change from production) Best Practices: 91 (🔴 down 7 from production) SEO: 92 (no change from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

wackerow

@pettinarip Overall looks good! Left one comment which appears to be a visual regression. If we patch that I think this is ready to be pulled in.

wackerow · 2024-07-30T21:58:50Z

src/components/Nav/Mobile/ExpandIcon.tsx

-    strokeWidth="2px"
-    display="inline-block"
-    stroke="currentColor"
+    className="relative inline-block h-6 w-6 stroke-current stroke-2 group-hover:text-primary-hover"


Is group-hover:text-primary-hover correct? Production does not use the primary color here on hover/pressing

So, this is prod:
https://github.com/user-attachments/assets/d223a229-0c69-46a9-ad9a-efa5226c2656

Edit: in the video you can see that we have a hover style on the icon.

Anyway, now that I think, it doesn't really matter, this is a mobile only menu where we are not going to have hover events. So, happy to remove it to make it cleaner.

🤔 Hm.. On production I'm seeing this effect only in nested menus that have been closed after opening (the + ends up colored). I suspect that is a design bug to begin with, with the intended effect being no colored svg's throughout (the svg shape change and menu folding/unfolding is enough of an effect)

With this PR, once a menu is activated, there is always one of these svg icons in primary color. And note on mobile "hover", it does change color when I tap on a menu, and doesn't go away when I lift my finger (Safari, iOS):

RPReplay_Final1722975035.mp4

This is nit-picky though, and as I noted, I think there is an inconsistency we could work out on the side anyways to polish up that particular styling, and don't see it as a blocker to this PR.

…s in the nav

wackerow

Looks good @pettinarip! Left a couple comments but going to pull this in

wackerow · 2024-08-06T20:18:59Z

src/components/Nav/Mobile/ExpandIcon.tsx

-    strokeWidth="2px"
-    display="inline-block"
-    stroke="currentColor"
+    className="relative inline-block h-6 w-6 stroke-current stroke-2 group-hover:text-primary-hover"


🤔 Hm.. On production I'm seeing this effect only in nested menus that have been closed after opening (the + ends up colored). I suspect that is a design bug to begin with, with the intended effect being no colored svg's throughout (the svg shape change and menu folding/unfolding is enough of an effect)

With this PR, once a menu is activated, there is always one of these svg icons in primary color. And note on mobile "hover", it does change color when I tap on a menu, and doesn't go away when I lift my finger (Safari, iOS):

RPReplay_Final1722975035.mp4

wackerow · 2024-08-06T20:19:45Z

src/components/Nav/Mobile/ExpandIcon.tsx

-    strokeWidth="2px"
-    display="inline-block"
-    stroke="currentColor"
+    className="relative inline-block h-6 w-6 stroke-current stroke-2 group-hover:text-primary-hover"


This is nit-picky though, and as I noted, I think there is an inconsistency we could work out on the side anyways to polish up that particular styling, and don't see it as a blocker to this PR.

wackerow · 2024-08-06T20:30:20Z

src/hooks/useNavMenuColorsTw.ts

+      1: {
+        subtext: isLight ? "text-gray-400" : "text-gray-400",
+        background: isLight ? "bg-white" : "bg-black",
+        activeBackground: isLight ? "bg-gray-150" : "bg-gray-700",
+      },
+      2: {
+        subtext: isLight ? "text-gray-400" : "text-gray-300",
+        background: isLight ? "bg-gray-150" : "bg-gray-700",
+        activeBackground: isLight ? "bg-gray-200" : "bg-gray-600",
+      },
+      3: {
+        subtext: isLight ? "text-gray-500" : "text-gray-300",
+        background: isLight ? "bg-gray-200" : "bg-gray-600",
+        activeBackground: isLight ? "bg-gray-100" : "bg-gray-700",
+      },
+      4: {
+        subtext: isLight ? "text-gray-700" : "text-gray-300",
+        background: isLight ? "bg-gray-300" : "bg-gray-700",
+        activeBackground: isLight ? "bg-gray-200" : "bg-gray-800",
+      },


Not sure if we want to tackle with this PR, but ideally we extract these as css variables in global.css—one under :root and one under [data-theme="dark"] for each with the same name, then add these as tokens in tailwind.config.ts.

I'm working on theming separately, so happy to just handle this as part of that process.

github-actions bot added dependencies 📦 Changes related to project dependencies tooling 🔧 Changes related to tooling of the project labels Jul 23, 2024

pettinarip commented Jul 23, 2024

View reviewed changes

src/components/Nav/Mobile/LvlAccordion.tsx Outdated Show resolved Hide resolved

wackerow mentioned this pull request Jul 24, 2024

migrate: useDisclosure hook #13459

Closed

wackerow reviewed Jul 24, 2024

View reviewed changes

src/hooks/useDisclosure.ts Outdated Show resolved Hide resolved

wackerow reviewed Jul 24, 2024

View reviewed changes

src/components/Nav/Mobile/index.tsx Outdated Show resolved Hide resolved

pettinarip added 11 commits July 25, 2024 19:44

initial mobile nav menu migration

7c60864

create custom useDisclosure hook and refactor state for mobile menu

00f5d3b

migrate nested accordions

07ac8bb

extend blues and oranges

004cbb0

cleanup

c6dced5

refactor: create MenuAccordion to isolate custom styles and logic

9cef30a

fix

da5a12a

use MenuButton in MenuFooter to be backwards compatible

69f4d16

revert changes on main sb file

9fef297

remove temp button and use the new shadcn version

572ab9e

named export hook

eba5968

github-actions bot added content 🖋️ This involves copy additions or edits documentation 📖 Change or add documentation translation 🌍 This is related to our Translation Program labels Jul 26, 2024

pettinarip force-pushed the shadcn-mobile-menu branch from 7bdd73a to eba5968 Compare July 26, 2024 08:56

pettinarip added 3 commits July 26, 2024 11:00

Merge branch 'shadcn-footer-link' into shadcn-mobile-menu

b9aa595

use custom useColorModeValue in mobile menu

fa4bd40

adjust outline styles to match button's focus colors

954d67e

pettinarip changed the base branch from shadcn-footer-link to dev July 26, 2024 11:40

pettinarip added the Status: Blocked 🛑 This is blocked label Jul 26, 2024

pettinarip marked this pull request as ready for review July 26, 2024 14:42

pettinarip requested review from corwintines and minimalsm as code owners July 26, 2024 14:42

pettinarip mentioned this pull request Jul 29, 2024

Shadcn migration - language picker #13530

Closed

6 tasks

pettinarip mentioned this pull request Jul 29, 2024

Shadcn migration - language picker #13531

Merged

6 tasks

pettinarip added 3 commits July 30, 2024 15:37

Merge branch 'dev' into shadcn-mobile-menu

337132a

remove duplicated dep

1fe1fd2

fix import

81370fe

pettinarip removed the Status: Blocked 🛑 This is blocked label Jul 30, 2024

wackerow requested changes Jul 30, 2024

View reviewed changes

Merge branch 'dev' into shadcn-mobile-menu

a1dfb28

pettinarip mentioned this pull request Aug 3, 2024

fix(useNav): remove unused fromPageParameter #13568

Merged

pettinarip added 5 commits August 5, 2024 16:10

Merge branch 'dev' into shadcn-mobile-menu

2123ddf

update bad link import

3d811c2

Merge branch 'dev' into shadcn-mobile-menu

3aa00fe

remove py from hamburger button to match the size of the other button…

bcb5df1

…s in the nav

fix spacing between buttons

b8c6f6a

wackerow approved these changes Aug 6, 2024

View reviewed changes

Merge branch 'dev' into shadcn-mobile-menu

a0a4760

wackerow merged commit 576e1ad into dev Aug 6, 2024
7 of 10 checks passed

wackerow deleted the shadcn-mobile-menu branch August 6, 2024 21:35

This was referenced Aug 7, 2024

Release candidate v8.9.0 #13595

Merged

Deploy v8.9.0 #13596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shadcn migration - mobile menu #13449

Shadcn migration - mobile menu #13449

pettinarip commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 30, 2024 •

edited

Loading

wackerow left a comment

wackerow Jul 30, 2024

pettinarip Jul 31, 2024 •

edited

Loading

wackerow Aug 6, 2024

wackerow Aug 6, 2024

wackerow left a comment

wackerow Aug 6, 2024

wackerow Aug 6, 2024

wackerow Aug 6, 2024

Shadcn migration - mobile menu #13449

Shadcn migration - mobile menu #13449

Conversation

pettinarip commented Jul 23, 2024 • edited Loading

Description

Related Issue

netlify bot commented Jul 30, 2024 • edited Loading

✅ Deploy Preview for ethereumorg ready!

wackerow left a comment

Choose a reason for hiding this comment

wackerow Jul 30, 2024

Choose a reason for hiding this comment

pettinarip Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

wackerow Aug 6, 2024

Choose a reason for hiding this comment

wackerow Aug 6, 2024

Choose a reason for hiding this comment

wackerow left a comment

Choose a reason for hiding this comment

wackerow Aug 6, 2024

Choose a reason for hiding this comment

wackerow Aug 6, 2024

Choose a reason for hiding this comment

wackerow Aug 6, 2024

Choose a reason for hiding this comment

pettinarip commented Jul 23, 2024 •

edited

Loading

netlify bot commented Jul 30, 2024 •

edited

Loading

pettinarip Jul 31, 2024 •

edited

Loading