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

Jump to bottom

Use section instead of header in dashboard pages #14873

Merged
merged 6 commits into from Oct 6, 2021
Merged

Use section instead of header in dashboard pages #14873

merged 6 commits into from Oct 6, 2021

Conversation

MohamedAsan
Copy link
Contributor

@MohamedAsan MohamedAsan commented Sep 30, 2021 •
edited

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update
  • Accessibility

Description

Change <header> to <section> to avoid having two banner landmarks and move it under

Related Tickets & Documents

Resolves #14840

QA Instructions, Screenshots, Recordings

Verified that there are no visual changes in both desktop(Chrome/Safari/Firefox) & mobile browsers(Chrome/Safari)
Axe Results

UI accessibility concerns?

Verified by using screen reader

Added/updated tests?

  • Yes
  • No, and this is why:
  • I need help with writing tests

@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Sep 30, 2021
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

@MohamedAsan
Copy link
Contributor Author

@aitchiss @cmgorton Kindly review this

@cmgorton
Copy link
Contributor

cmgorton commented Oct 2, 2021

@MohamedAsan someone will likely review your PR after the weekend. We do not typically work on the weekends 😊Thanks for your contribution so far.

@msarit
Copy link
Contributor

msarit commented Oct 4, 2021

@MohamedAsan Hello! 👋🏾
Could you please merge main into your branch? It will resolve the TravisCI failure 👍🏾

@MohamedAsan
Copy link
Contributor Author

Thank you @benhalpern

@rhymes rhymes requested a review from aitchiss October 5, 2021 08:34
@rhymes
Copy link
Contributor

rhymes commented Oct 5, 2021 •
edited

Hi @MohamedAsan, there's now a conflict, can you take a look? Something must have been merged ahead of this, sorry for that :( and thank you!

aitchiss
aitchiss suggested changes Oct 5, 2021
View reviewed changes
Copy link
Contributor

@aitchiss aitchiss left a comment

Choose a reason for hiding this comment

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

Thanks for taking this one on! It's looking good, but left a note about some further tweaks I think we need to make - let me know if you have any questions at all!

app/views/dashboards/followers.html.erb Outdated
Comment on lines 6 to 9
<section class="crayons-layout" aria-labelledby="header-title">
<h1 class="p-2 pb-0 m:p-0 crayons-title" id="header-title">Dashboard &raquo; Followers</h1>
<%= render "actions_mobile" %>
</header>
</section>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to make a few further tweaks to get the landmarks all in the place we want -

  • This section is part of the main content of the page, so let's move it inside the main
  • Excellent plan labelling the section, but I don't think this is necessary in all cases - when a section is labelled, it's added to the landmark regions for screen reader users. In this case I don't think the section is sufficiently important/standalone to merit that, so we can just remove the aria-labelledby

The same can be applied to the other areas below too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. Learnt a few things here :) Honestly, I don't know much about accessibility, and I was just resolving the errors shown by axe. I have made the changes as requested. Kindly let me know if those are okay

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making those changes! It's great to see those errors disappearing, and this makes things much better for landmark navigation for screen reader users 😄

If you want to check out some examples of how screen readers allow for landmark navigation, there's a nice article summarising this on w3 - with your changes, people will now be able to get to the main content more easily, and not be confused by the duplicate header! 🌟

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot 😄

@pr-triage pr-triage bot added PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 5, 2021
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-changes-requested bot applied label for PR's where reviewer requests changes labels Oct 6, 2021
aitchiss
aitchiss approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@aitchiss aitchiss left a comment

Choose a reason for hiding this comment

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

Looking great!

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 6, 2021
rhymes
rhymes approved these changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@rhymes rhymes left a comment

Choose a reason for hiding this comment

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

Awesome work @MohamedAsan! Thank you :)

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 6, 2021
@rhymes rhymes merged commit cceb175 into forem:main Oct 6, 2021
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Oct 6, 2021
@cmgorton
Copy link
Contributor

cmgorton commented Oct 6, 2021

Thanks so much for your work here @MohamedAsan !

@MohamedAsan
Copy link
Contributor Author

Thanks a lot for the opportunity to learn and contribute @aitchiss @cmgorton @rhymes @benhalpern @msarit 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhymes rhymes rhymes approved these changes

@aitchiss aitchiss aitchiss approved these changes

Assignees
No one assigned
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dashboard pages shouldn't have two banner landmarks
7 participants
@MohamedAsan @CLAassistant @cmgorton @msarit @rhymes @aitchiss @benhalpern