Fix/resume headings#356
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses accessibility issue #347 in the Resume section by correcting heading level hierarchy so headings no longer skip from h2 to h4.
Changes:
- Update each resume role title heading from
h4toh3to maintain semantic heading order under the Resume section’sh2. - Update Resume section CSS to style
h3role headings consistently and narrow the.resume-card__section h3rule to direct children only. - Adjust mobile/narrow layout CSS variables for role title sizing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/sections/resume/ResumeSection.ts |
Changes role headings to h3 to fix the heading-level skip. |
src/sections/resume/ResumeSection.css |
Adds h3 selectors and refines section heading targeting to avoid unintended styling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <li class="resume-card__item" role="listitem" aria-labelledby=${roleTitleId} aria-describedby=${ariaDescribedBy}> | ||
| <div class="resume-card__item-header"> | ||
| <h4 id=${roleTitleId}>${role.title}</h4> | ||
| <h3 id=${roleTitleId}>${role.title}</h3> |
There was a problem hiding this comment.
So i feel like this is okay because .profile-grid .resume-card__section h3 is resume__card h3. There are no other h3 in resume card, let me know what you think @NoelDeMartin
There was a problem hiding this comment.
Looking at the page as it was before, I think it makes more sense to leave it as h3 yes. Although given the dynamic nature of this components, maybe there is a situation where <h4> would make more sense, I'm not sure :/. In any case, what is 100% sure is that an <h4> should go after an <h3>, without skipping levels. And seeing how this component is used currently, it sits under an <h3>.
NoelDeMartin
left a comment
NoelDeMartin
left a comment
There was a problem hiding this comment.
This looks good, I'm not merging because I'm not sure how we want to proceed with the PRs until we start using the staging branch. But I think this can be merged if you want.
Ticket