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

🎨 [#1378] Header multiple navigation (mobile and desktop) #591

Merged

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Apr 20, 2023

new mobile header designs + 2 new desktop menus:
https://projects.invisionapp.com/share/B3137YU0PZY2#/screens?browse

In order to view the styles correctly for review: turn off the Django CMS-toolbar
(http://127.0.0.1:8000/?toolbar_off)

adds feature to issue mobile: https://taiga.maykinmedia.nl/project/open-inwoner/task/1378
also adds feature to issue for desktop header: https://taiga.maykinmedia.nl/project/open-inwoner/task/1377
and adds feature to issue for indicator + dot: https://taiga.maykinmedia.nl/project/open-inwoner/task/1398

📲 Mobile menu:

  • visually looks like 1 list of all pages + subpages for Onderwerpen
  • no stickies: menu + search bar must scroll out of view when menu is closed
  • search bar is visible in opened mobile menu
  • message/plans notifications visible
  • some new icons
    Mobile menu works in all browsers the same

🖥️ Desktop menu:

  • two separate menus for 1) Onderwerpen and 2) is_authenticated
  • subpages must not be visible on hover, only on toggle/click
  • message/plans notifications visible
  • needs to be accessible (close on Escape and out-of-Focus, open on Tab and open on 1 click)

🔔 Note:
NB: in Firefox desktop the Tab can't reach the submenu-items unless accessibility.tabfocus is set to number=7 in the about:config settings - which may be an issue on Mac only: https://support.mozilla.org/en-US/questions/1278793
NB: In Safari Tab won't work if user forgets to turn on accessibility (Settings > Advanced > Turn on Tab...).

@jiromaykin jiromaykin changed the title 🎨 [#1309] Created branch for mobile menu change 🎨 [#1309] Mobile-header navigation (no-hamburger) Apr 20, 2023
@jiromaykin jiromaykin changed the title 🎨 [#1309] Mobile-header navigation (no-hamburger) 🎨 [#1378] Mobile-header navigation (no-hamburger) Apr 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2023

Codecov Report

Merging #591 (c80f731) into develop (325d753) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head c80f731 differs from pull request most recent head 9ddb4d1. Consider uploading reports for the commit 9ddb4d1 to get more accurate results

@@             Coverage Diff             @@
##           develop     #591      +/-   ##
===========================================
+ Coverage    96.39%   96.50%   +0.10%     
===========================================
  Files          574      573       -1     
  Lines        19811    19773      -38     
===========================================
- Hits         19096    19081      -15     
+ Misses         715      692      -23     
Impacted Files Coverage Δ
...pen_inwoner/components/templatetags/header_tags.py 100.00% <100.00%> (ø)
src/open_inwoner/plans/tests/test_views.py 96.54% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch 3 times, most recently from dc92b3e to 2962687 Compare May 2, 2023 08:19
@jiromaykin jiromaykin changed the title 🎨 [#1378] Mobile-header navigation (no-hamburger) 🎨 [#1378] Header multiple navigation (mobile and desktop) May 2, 2023
@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch from 893012d to 4a9c982 Compare May 4, 2023 00:30
@jiromaykin jiromaykin marked this pull request as ready for review May 4, 2023 13:06
@jiromaykin jiromaykin requested a review from vaszig May 4, 2023 13:45
@jiromaykin
Copy link
Contributor Author

@vaszig : these are the 3 new menus, all static for the moment.
I have added A LOT of console.logs to see if I can debug the problems in Safari - but after review I will remove all these logs.
For now you can test the menus in Chrome/Firefox etc. And see if they respond to Tab keys and Escape and if it isn't a mess.
These 3 menus are only for mobile and desktop (not for tablet width yet) - it was impossible to isolate the header in just mobile and desktop so that's why this PR tackles 3 issues at once.
And: tabs/escape work on Safari, but.. onClick events on the Desktop menu do not work in any Safari version yet 😢

vaszig

This comment was marked as resolved.

@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch 3 times, most recently from e269180 to ac46aee Compare May 9, 2023 14:17
@jiromaykin
Copy link
Contributor Author

@Bartvaderkin this is the big one... up for review.
I handled 3 different issues in one PR because they all needed the new menus in order to work. Let me know if I need to change anything to make it easier for you to turn this into a dynamic menu.

@Bartvaderkin
Copy link
Contributor

@jiromaykin the PR got outdated and has a merge conflict, can you rebase on develop and push again?

@jiromaykin
Copy link
Contributor Author

@jiromaykin the PR got outdated and has a merge conflict, can you rebase on develop and push again?

@ bart Okay I will.

@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch from c80f731 to 9ddb4d1 Compare May 11, 2023 11:06
@jiromaykin jiromaykin force-pushed the feature/1378-mobile-header-navigation-no-hamburger branch from 9ddb4d1 to 6f4e15d Compare May 12, 2023 09:15
@alextreme alextreme merged commit 50768aa into develop May 12, 2023
@alextreme alextreme deleted the feature/1378-mobile-header-navigation-no-hamburger branch May 12, 2023 13:49
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