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

♿ [#2625] Add mobile design for keeping accessibility functions available on small screens #1470

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

jiromaykin
Copy link
Contributor

@jiromaykin jiromaykin commented Oct 24, 2024

issue: https://taiga.maykinmedia.nl/project/open-inwoner/task/2625

  • functionality visible on desktop must remain available when zooming in 200%
  • skip links are not allowed in Navs, so I moved it higher up
  • when desktop users zoom in > 200% they will see the mobile view, but usually do not have typical mobile touch functions like "pinching" etc. so they still need the 'Vergroten' function

Design: https://www.figma.com/design/iKGhWhstaLIlFSaND2q7cE/OIP---Designs-(new)?node-id=1664-13773&m=dev

A11Y docs from NL Design-System: : https://nl-design-system.github.io/utrecht/storybook/?path=/docs/css_css-skip-link--docs

Skiplink on Desktop and Mobile becomes visible as very first Tabbable focus-visible element:

Screenshot 2024-11-05 at 10 12 28

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.31%. Comparing base (5aba3cc) to head (aa2b195).
Report is 3 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1470   +/-   ##
========================================
  Coverage    94.31%   94.31%           
========================================
  Files         1066     1066           
  Lines        40063    40065    +2     
========================================
+ Hits         37785    37787    +2     
  Misses        2278     2278           

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

@jiromaykin jiromaykin force-pushed the feature/2625-show-accessibility-header-mobile branch from ce242bd to ac1e40f Compare November 4, 2024 09:51
@jiromaykin jiromaykin changed the title ♿ [#2625] Add mobile design for keeping accessibilty header on small screens ♿ [#2625] Add mobile design for keeping accessibility functions available on small screens Nov 5, 2024
@jiromaykin jiromaykin force-pushed the feature/2625-show-accessibility-header-mobile branch 2 times, most recently from 801eca9 to dc64266 Compare November 5, 2024 09:53
@jiromaykin jiromaykin force-pushed the feature/2625-show-accessibility-header-mobile branch from 2cfd57c to 98e1531 Compare November 5, 2024 18:59
@jiromaykin jiromaykin marked this pull request as ready for review November 5, 2024 19:00
@jiromaykin
Copy link
Contributor Author

jiromaykin commented Nov 5, 2024

@pi-sigma @swrichards How to review this PR:

  • terminate all your NPM processes
  • do a npm ci --legacy-peer-deps = this will retrieve the latest packages, like the updated Utrecht Components and including my updated package with new design-tokens
  • npm run build
  • runserver
  • clear browser caches
  • click and see if accessibility functionality is working in the opened mobile menu, like: if Vergroten/Verkleinen and its attributes are still toggling correctly (for the Speech stuff: either use Browserstack or use official Google Chrome browsers on Linux)
  • you can reach the Skiplink by reverse(Shift) Tabbing, (if it is not the first element in your visible focus anymore).
  • +codereview: I had to change some of the Tests, since it's no longer needed to show a Search bar when mobile menu is opened.

@swrichards
Copy link
Collaborator

Thanks for the review steps @jiromaykin -- very helpful!

I see the a11y buttons and they're functional (enlarge, read aloud work as expected, at least for the latter with the known caveats for Linux). I can also reach the skiplink. The styling of "Inloggen" en "Zoeken" does look slightly off to me compared to the design:

Screenshot from 2024-11-06 09-25-47

Is that expected? I went through the steps outlined above (and did another collectstatic for good measure).

Copy link
Collaborator

@swrichards swrichards left a comment

Choose a reason for hiding this comment

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

Just a question and small points, thanks.

search_forms = [
form
for form in doc.find("form").items()
if form.find("button[title='Zoeken']")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick I guess, but: can't we simply do something like doc.find("form:has(> button[title='Zoeken'])")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did at first! But then I got the error "cssselect.parser.SelectorSyntaxError: Expected an argument, got <DELIM '[' at 15>..." and it seems due to cssselect, which doesn’t support all the same selectors as the jQuery-like PyQuery. Specifically, it seems to not support :has() with attribute selectors directly.

To get around this limitation, I tried an alternative approach: first find all forms on the page, then filter them programmatically to check if they contain a button[title='Zoeken'] with a simple CSS attribute selector that would work in all browser engines.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shame that the selector support appears to be limited, but no worries, thanks for the explanation!


This will include the `Open-Inwoner-Design-Tokens`_ subdirectory. When all is built and run this is where the OIP design tokens CSS will be generated. When this repository gets updated, it needs to be pulled again.

.. _Open-Inwoner-Design-Tokens: https://github.com/maykinmedia/open-inwoner-design-tokens
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might not be a bad idea to still include a reference to this repo? Not in these steps indeed, but just as a note (perhaps in step 5, "Note the project makes use of the open-inwoner-design-tokens package, which contains the project's design tokens for the NL Design System."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, I could, though programmatically it has the same importance as all other NPM packages needed for this project to work, so NLDS package is just another Node module now.
This Readme contained a remnant from when we were still using some subdirectory packages.

@jiromaykin
Copy link
Contributor Author

[...]The styling of "Inloggen" en "Zoeken" does look slightly off to me compared to the design: [...]
Is that expected? I went through the steps outlined above (and did another collectstatic for good measure).

@swrichards Oh no's that screenshot doesn't look correct at all.
If you did a proper NPM build and (perhaps multiple) browser cache reloads, it should look like this:
in your screenshot e.g. I am missing the new Home icon, FAQ icon, and also the new Close button:

a11y-close

@swrichards
Copy link
Collaborator

[...]The styling of "Inloggen" en "Zoeken" does look slightly off to me compared to the design: [...]
Is that expected? I went through the steps outlined above (and did another collectstatic for good measure).

@swrichards Oh no's that screenshot doesn't look correct at all. If you did a proper NPM build and (perhaps multiple) browser cache reloads, it should look like this: in your screenshot e.g. I am missing the new Home icon, FAQ icon, and also the new Close button:
a11y-close

This is now fixed (presumably due to working from a stale branch ref on my end).

@swrichards swrichards closed this Nov 7, 2024
@swrichards swrichards reopened this Nov 7, 2024
@swrichards swrichards self-requested a review November 7, 2024 13:15
@alextreme alextreme merged commit 282365d into develop Nov 11, 2024
39 checks passed
@alextreme alextreme deleted the feature/2625-show-accessibility-header-mobile branch November 11, 2024 09:07
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