Skip to content

feat: APP-374 header navigation toggle#2581

Merged
r41ph merged 15 commits intodevfrom
fix-APP-374-mobile-navigation-toggle
Feb 11, 2025
Merged

feat: APP-374 header navigation toggle#2581
r41ph merged 15 commits intodevfrom
fix-APP-374-mobile-navigation-toggle

Conversation

@r41ph
Copy link
Contributor

@r41ph r41ph commented Jan 20, 2025

Description

https://regennetwork.atlassian.net/browse/APP-374

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. https://deploy-preview-2581--regen-marketplace.netlify.app/
  2. check the navigation dropdown and the hamburger icon work on small screens as well as in desktop screens.
  3. Both the hamburger icon navigation and the dropdown navigation should now:
  • Close when the other one is opened.
  • Toggle when clicked again.
  • Close when open and a click occurs outside of them.
  • Close when a nav item is clicked.

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

@netlify
Copy link

netlify bot commented Jan 20, 2025

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit b69d1c4
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/67ab3d20f1fd790009e50663
😎 Deploy Preview https://deploy-preview-2581--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@netlify
Copy link

netlify bot commented Jan 20, 2025

Deploy Preview for terrasos canceled.

Name Link
🔨 Latest commit 3534a12
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/678e8b086d399b0008faf86a

@netlify
Copy link

netlify bot commented Jan 20, 2025

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit b69d1c4
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/67ab3d20760d8c00087afd88
😎 Deploy Preview https://deploy-preview-2581--terrasos.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@r41ph
Copy link
Contributor Author

r41ph commented Jan 20, 2025

@erikalogie @S4mmyb see testing instructions

@r41ph r41ph requested a review from blushi January 20, 2025 18:01
@netlify
Copy link

netlify bot commented Jan 21, 2025

marie left a comment:

recording

Not sure if that's really an issue since the hamburger menu will likely appear on touch screens where this is not possible, but just in case, I wanted to double check whether we can do something about it or not

Browser metadata
Path:      /dashboard/projects
Browser:   Chrome 131.0.0.0 on Mac OS 10.15.7
Viewport:  1018 x 884 @1x
Language:  fr-FR
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@r41ph
Copy link
Contributor Author

r41ph commented Jan 21, 2025

Not sure if that's really an issue since the hamburger menu will likely appear on touch screens where this is not possible..

I think this is a good catch. I believe it’s important to consider edge cases where users resize their browser window. I have updated the code to detect whether the screen is touch and handle the navigation based on that, instead of working with the window size. This should make the experience more consistent.

@erikalogie
Copy link
Collaborator

LGTM

@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch 2 times, most recently from 87d1785 to 447306c Compare January 22, 2025 18:03
@S4mmyb
Copy link
Member

S4mmyb commented Jan 23, 2025

LGTM

@blushi
Copy link
Member

blushi commented Jan 28, 2025

Looks like when clicking on one of the user menu items (eg portfolio) after initial page loading, the dropdown menu appears in the bottom right corner for a few seconds (see video below).
Looks related to https://regennetwork.atlassian.net/browse/APP-28 but so far it was hard to reproduce, now with the changes in this PR, I can reproduce it easily using the instructions just above, so maybe it would be worth trying to tackle that?

@netlify
Copy link

netlify bot commented Jan 28, 2025

marie left a comment:

recording

Browser metadata
Path:      /dashboard/portfolio
Browser:   Chrome 131.0.0.0 on Mac OS 10.15.7
Viewport:  1429 x 824 @1x
Language:  fr-FR
Cookies:   Enabled

Open in BrowserStack

Open Deploy Preview · Mark as Resolved

@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch from 447306c to 6f47a65 Compare January 30, 2025 10:05
@r41ph
Copy link
Contributor Author

r41ph commented Jan 30, 2025

@blushi see updated code please. I have refactored the header navigation dropdown to not use Popover and also amend some semantic issues. The dropdown now shouldn't jump anymore to the bottom right corner. I have also updated the MobileMenu so we don't have duplicated markup in the DOM.

In some route changes like between /, /projects, /dashboard or /admin/dashboard I see like a flickering, like the whole app re-renders for some reason. I think we should look into this, have you noticed? wdyt?

@r41ph r41ph requested a review from blushi January 30, 2025 10:24
@blushi
Copy link
Member

blushi commented Feb 3, 2025

@blushi see updated code please. I have refactored the header navigation dropdown to not use Popover and also amend some semantic issues. The dropdown now shouldn't jump anymore to the bottom right corner. I have also updated the MobileMenu so we don't have duplicated markup in the DOM.

In some route changes like between /, /projects, /dashboard or /admin/dashboard I see like a flickering, like the whole app re-renders for some reason. I think we should look into this, have you noticed? wdyt?

Thanks for refactoring this!

Yeah I don't think the flickering was there before, so we should probably have a look.

A few remaining issues I noticed:

  • the dropdown menus are too large with some extra left padding
    image

  • when opening the menu, the transition was a bit longer before

  • if I'm on the homepage then go to my dashboard, then go back to homepage (using regen market top left logo) then sometimes I get this when opening the user menu:

image

@r41ph
Copy link
Contributor Author

r41ph commented Feb 3, 2025

the dropdown menus are too large with some extra left padding

@blushi I'm not seeing the extra padding nor the missing colours. What browser are you using? Have you tried clearing the cache and doing hard refresh to see if it still happens?

when opening the menu, the transition was a bit longer before

I'll review animation timing

@blushi
Copy link
Member

blushi commented Feb 3, 2025

the dropdown menus are too large with some extra left padding

@blushi I'm not seeing the extra padding nor the missing colours. What browser are you using? Have you tried clearing the cache and doing hard refresh to see if it still happens?

Yeah this doesn't help, using Chrome. I see the same on Brave, Firefox and Safari

@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch from b7a1fe2 to caff73c Compare February 3, 2025 15:30
@r41ph
Copy link
Contributor Author

r41ph commented Feb 3, 2025

@blushi I have updated the animation timing and also fixed the padding issue. Re the missing colours, I can't reproduce it in any browser anymore after seeing it in Brave once. Is it still happening to you in the latest deploy preview?

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

image
the user menu layout at the top is wrong now
versus on dev:
image

The animation feels less "smooth" than before, especially when the dropdown menu closes. Before the full dropdown was fading out, now it's using translateY/scale which looks a bit different.

@blushi
Copy link
Member

blushi commented Feb 4, 2025

I can't reproduce it in any browser anymore after seeing it in Brave once. Is it still happening to you in the latest deploy preview?

I haven't been able to reproduce this so far.

@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch from caff73c to 791d140 Compare February 5, 2025 14:33
@r41ph
Copy link
Contributor Author

r41ph commented Feb 5, 2025

The animation feels less "smooth" than before, especially when the dropdown menu closes. Before the full dropdown...

@blushi, I have updated the timing again and remove the `translate(), wdyt?
@erikalogie @S4mmyb do you want to have another look?

@r41ph r41ph requested a review from blushi February 5, 2025 14:43
@S4mmyb
Copy link
Member

S4mmyb commented Feb 5, 2025

Hey @r41ph , this looks good on mobile. One thing I noticed on desktop, though, is that for the user profile dropdown you can open the drop-down on-hover and collapse it with a click. But you can't get it to drop down again on-click, you need to move the mouse and hover on it again.

Recording.2025-02-05.121616.mp4

@r41ph
Copy link
Contributor Author

r41ph commented Feb 6, 2025

@S4mmyb the menu shouldn't close on click now (on desktop)

@erikalogie
Copy link
Collaborator

LGTM

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

tACK

@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch from 309708f to 0e69e12 Compare February 11, 2025 09:25
@r41ph r41ph force-pushed the fix-APP-374-mobile-navigation-toggle branch from 0e69e12 to b69d1c4 Compare February 11, 2025 12:05
@r41ph r41ph merged commit 77cf40a into dev Feb 11, 2025
18 checks passed
@r41ph r41ph deleted the fix-APP-374-mobile-navigation-toggle branch February 11, 2025 12:18
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.

4 participants