Skip to content

Conversation

@knowler
Copy link
Contributor

@knowler knowler commented Jan 31, 2026

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 corresponding article ARIA role is not a navigational landmark, so this change will only impact users when they are traversing through the page.

Co-authored-by: Rshig <90143161+rshigg@users.noreply.github.com>
@vercel
Copy link

vercel bot commented Jan 31, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Jan 31, 2026 5:56pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
npmx-lunaria Ignored Ignored Jan 31, 2026 5:56pm
docs.npmx.dev Skipped Skipped Jan 31, 2026 5:56pm

Request Review

@rshigg
Copy link
Contributor

rshigg commented Jan 31, 2026

Any reason to withhold removing the aside and ancestor article? I think this is a good opportunity to get that all sorted at once

Copy link
Contributor Author

@knowler knowler Jan 31, 2026

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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 the README heading 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 > article element 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. The article ARIA 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.

@danielroe danielroe enabled auto-merge January 31, 2026 17:59
@danielroe danielroe added this pull request to the merge queue Jan 31, 2026
Merged via the queue into npmx-dev:main with commit 876aaee Jan 31, 2026
13 checks passed
@knowler knowler deleted the package-readme-article branch January 31, 2026 18:04
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.

3 participants