-
-
Notifications
You must be signed in to change notification settings - Fork 144
refactor: use an <article> element for for package readme
#524
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
Conversation
Co-authored-by: Rshig <90143161+rshigg@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Any reason to withhold removing the aside and ancestor article? I think this is a good opportunity to get that all sorted at once |
app/pages/[...package].vue
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m having second thoughts. I’m thinking this <div> that contains the README contents should be the <article> element, since our “README” heading isn’t a part of that document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do that, then I’ll probably remove the top-level <article> for sure since they’ll probably have the same name which might be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b671b9a I made the change here. I’m still considering removing the top-level <article> and <aside> elements. Taking sometime to think on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rshigg So, I think I’m done here and these are the changes:
- Instead of replacing the
<section>with theREADMEheading with an<article>element, I’ve used it to wrap the readme contents itself, which were wrapped in a<div>. This is because our heading is not a part of that document. - I considered removing
main > articleelement because it’d likely have a similar name to the README one, but I’ve left it because it really doesn’t have a whole lot of impact. ThearticleARIA role is really only exposed in macOS VoiceOver which has very low usage in comparison to other screen readers. So, you could say this whole PR was negligible, but we are using more semantic HTML now, so that’s at least a plus. - I did remove the
<aside>element used for the (presentational) sidebar.
As discussed in #503 (comment) and #503 (comment), @rshigg and I noticed that a package’s readme would be better wrapped with an
<article>element as it’s a standalone document. It’s alright that it’s a descendant of another<article>element (which wraps the page’s main contents).Worth noting that an
<article>element and its correspondingarticleARIA role is not a navigational landmark, so this change will only impact users when they are traversing through the page.