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

Improve meaningful sequence in report/around page #5354

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lucascumsille
Copy link
Contributor

@lucascumsille lucascumsille commented Feb 5, 2025

Fixes: https://github.com/mysociety/societyworks/issues/4560

Replaces: #5208

Unlike the previous PR#5208 I thought that a better approach to include the icons would be to use HTML templates instead of copy and pasting the SVG. See commit here

Preview

Screen.Recording.2025-02-06.at.07.01.49.mov

Preview Mobile

Screen.Recording.2025-02-06.at.07.03.19.mov

Pending from PR #5208

This template seems very similar to the non-mobile one, I can combine them later.
#5208 (comment)

  • templates/web/base/report/display_tools_mobile.html is quite

@lucascumsille lucascumsille force-pushed the 4560-meaningful-sequence-v2 branch from 7219efc to eb3bd29 Compare February 5, 2025 09:50
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.05%. Comparing base (61e7964) to head (6360815).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5354      +/-   ##
==========================================
+ Coverage   82.42%   85.05%   +2.63%     
==========================================
  Files         416      373      -43     
  Lines       32925    27642    -5283     
  Branches     5289     5289              
==========================================
- Hits        27137    23510    -3627     
+ Misses       4225     2570    -1655     
+ Partials     1563     1562       -1     

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

- This change will make it easier to control the icon size by passing it as an argument.
- It will also minimise issues with passing the contrast test, where currently the sprite has a fixed colour that doesn't necessarily pass the contrast test, and it's quite hard to read against light backgrounds. Now, the icons use currentColor, so it inherits the value of the parent.
@lucascumsille lucascumsille force-pushed the 4560-meaningful-sequence-v2 branch from eb3bd29 to a22092c Compare February 5, 2025 10:12
@lucascumsille
Copy link
Contributor Author

lucascumsille commented Feb 5, 2025

A couple of notes from the previous PR:
#5208

if ($('#key-tools-mobile a.js-feed').length) {

This change shouldn't be made - the js-feed is still in key-tools on any page it is used (not a report page), and needs to moved from there to sub_map_links.
And I'm afraid that sub_map_links uses a background image, so you then end up with two RSS feed icons being shown.
#5208 (comment)

I solved this issue in this PR. See here:

Screenshot 2025-02-05 at 10 18 22

- Added aria-expanded.
- Increase visibility of the drawer on mobile by moving it to the top of the screen.
- Added close button to drawer.
- If drawer is opened using keyboard then the focus jumps to the first element inside the drawer.
This commit should minimise the confusion for assistive device users. Currently, the way components are presented and the focus order is not linear; after they have travelled through the map, they are presented with the 'Can't use the map?' link and then go directly to the #key-tools component sitting at the bottom of the page, skipping the filters and report list. To respect the page sequence, the user should go directly to the report filter and then to the report list. The downside of this approach is that users who want to use the #key-tools would have to jump through a whole list of reports before being able to get to the #key-tools. The solution in this commit is to add a link right after the 'Can't use the map?' that gives the user some context about the mismatch between the reading sequence and also the opportunity to skip the #key-tools in case they just want to jump the next element which would be the report filters.
@lucascumsille lucascumsille force-pushed the 4560-meaningful-sequence-v2 branch from 85e4c8d to 6360815 Compare February 5, 2025 10:53
@lucascumsille lucascumsille marked this pull request as ready for review February 6, 2025 07:19
@lucascumsille lucascumsille requested a review from dracos February 6, 2025 07:19
@lucascumsille
Copy link
Contributor Author

lucascumsille commented Feb 6, 2025

@dracos I made the changes requested in #5208 so I split the commits into 4:

1.A commit to switch from external to inline SVGs, no other changes (that would also involve e.g. fixing the issue where the Feed link is moved to sub_map_links).
2.A commit to add the close button to the small drawer (and anything else related to that, but nothing more).
3.The skip button on around page if needed.
4.Changes to have a separate key-tools for desktop than mobile (fixing the issue with placement, not working for non-staff, etc)

In the review I included the latest comments you left in #5208

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.

1 participant