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

[FMS] Accessibility improvements #4312

Merged
merged 8 commits into from
Mar 3, 2023
Merged

[FMS] Accessibility improvements #4312

merged 8 commits into from
Mar 3, 2023

Conversation

lucascumsille
Copy link
Contributor

@lucascumsille lucascumsille commented Mar 1, 2023

I tried splitting the commits so I can drop them as needed.

Heading hierarchy

Fixes: https:/mysociety/societyworks/issues/3428
As we spoke, the repeating H1 where not issue, because only only is visible.
Related commits:

Around Page

Related, but more UI related. I got rid of the yellow chevron background on this commit:
[FMS] Erased odd chevron bakground click-the-map element
Screenshot 2023-02-28 at 10 51 37

Report page

Greenwich Improvements to navbar.

The mobile navbar was overlapping the desktop one. Leaving a very small clickable area.

Close mobile icon was white, therefore invisible.

Mixin button-variant

Fixes: https:/mysociety/societyworks/issues/3333

This one separate the focus state with the hover effect. Because all cobrands will be using the default mixing yellow, we could have some cobrands complaining about their styleguide, but I still thinks this is more accessible of what we currently have.
[FMS] Added focus state to button-variant mixin

This one adds an outline to the focus state in case we wanted to make it more obvious. But we can drop it if you think is too much.
[FMS] Added Outline property to button-variant mixin

Focus state for local images

Fixes: https:/mysociety/societyworks/issues/3298
Screenshot 2023-03-01 at 07 47 26

[FMS] Added focus state to local alert images page

I hope it's easy to understand, but let me know if you need me to clarify anything.

@lucascumsille lucascumsille marked this pull request as ready for review March 1, 2023 08:53
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Merging #4312 (d85f22c) into master (ccb8c13) will decrease coverage by 11.16%.
The diff coverage is n/a.

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

@@             Coverage Diff             @@
##           master    #4312       +/-   ##
===========================================
- Coverage   83.14%   71.98%   -11.16%     
===========================================
  Files         358       41      -317     
  Lines       26597     4576    -22021     
  Branches     4133        0     -4133     
===========================================
- Hits        22113     3294    -18819     
+ Misses       3239     1282     -1957     
+ Partials     1245        0     -1245     
Impacted Files Coverage Δ
web/js/front.js 82.25% <0.00%> (-1.62%) ⬇️
perllib/Utils.pm
perllib/FixMyStreet/SendReport/Triage.pm
perllib/FixMyStreet/DB/ResultSet/Problem.pm
perllib/FixMyStreet/PhotoStorage/S3.pm
...erllib/FixMyStreet/App/Controller/Admin/Reports.pm
perllib/FixMyStreet/DB/ResultSet/Comment.pm
perllib/FixMyStreet/Roles/ContactExtra.pm
perllib/FixMyStreet/DB/Result/Body.pm
perllib/FixMyStreet/DB/Result/Role.pm
... and 308 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

All looks good - hopefully my comments all make sense, let me know if not!

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
web/cobrands/sass/_mixins.scss Outdated Show resolved Hide resolved
@lucascumsille lucascumsille force-pushed the 3428-heading-order branch 2 times, most recently from 0bdd09d to ef96303 Compare March 2, 2023 07:20
@lucascumsille
Copy link
Contributor Author

Thanks for the feedback @dracos =). I implemented everything stated above. How you prefer me to rebase this? All the commits that are related into one?

One extra thing I noticed was the Button for the Dropzone on Fixamingata. It wasn't accessible enough so I changed the color for the button to black:

Screenshot 2023-03-02 at 06 58 45

Screenshot 2023-03-02 at 06 52 01

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

One comment to remove h2.static is all, plus there's a map file committed by mistake.
Would be great if, in general, fixes on a PR could be committed as extra 'fixup' commits applying to the commits they are fixing, which makes it much easier to then re-review the changes - let me know if you'd like more information on that :)
In terms of squashing - yes, I would put all the updates heading in one commit, all the around page ones in one commit, the rest all make sense as separate commits, I think?

web/cobrands/sass/_base.scss Outdated Show resolved Hide resolved
@lucascumsille
Copy link
Contributor Author

Thanks @dracos I'll do the rebase squashing related commits. I'll include a fixup getting rid of the h4.static that is not being used first.
Thanks for adding your commit =)

@lucascumsille
Copy link
Contributor Author

@dracos I just pushed a --fixup getting read of the h2.static element. Let me know if you are happy with the changes and I'll rebase =)

Copy link
Member

@dracos dracos left a comment

Choose a reason for hiding this comment

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

The map file is still there needs removing as part of the rebase as well. Once you've tidied it up, let me have a final look before merging, thanks :)

@dracos dracos merged commit ea110e2 into master Mar 3, 2023
@dracos dracos temporarily deployed to github-pages March 3, 2023 17:50 — with GitHub Pages Inactive
@lucascumsille lucascumsille temporarily deployed to github-pages March 6, 2023 08:58 — with GitHub Pages Inactive
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.

2 participants