-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
feat(seo): update headings for user profiles #3530
feat(seo): update headings for user profiles #3530
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3530 +/- ##
==========================================
+ Coverage 67.37% 67.40% +0.02%
==========================================
Files 433 433
Lines 13707 13707
Branches 2467 2467
==========================================
+ Hits 9235 9239 +4
+ Misses 4426 4422 -4
Partials 46 46 ☔ View full report in Codecov by Sentry. |
Passing run #5569 ↗︎
Details:
Review all test suite changes for PR #3530 ↗︎ |
@@ -127,6 +127,7 @@ export const MemberProfile = ({ user, docs }: IProps) => { | |||
</Flex> | |||
<Box sx={{ flexDirection: 'column' }} mb={3}> | |||
<Heading | |||
as="h1" |
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.
question: Do we want to introduce any tests that verify this document structure? This would prevent us from accidentally introducing a regression
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.
This is a good idea but there are different ways of testing. We could test each component implementation to validate the html tags used. We could also test the hierarchy of a full page. That would be better IMO for a semantic sense and it removes coupling between tests and code. The ideal way would be to test / assert the semantic of the html of the app but I don't know any way of doing that.
🎉 This PR is included in version 1.183.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.182.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
PR Checklist
PR Type
Description
What this PR does
This PR update all Heading tags in the User (members and others) profile to specific html heading tags from the default h2 to h1, h2 or h3. It should respect a correct hierarchical structure. To sum up, all tabs are now confirmed h2, and every other heading in the card is now a h3 and the name of the user is now a h1.
Git Issues
Helps to close #3424
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes
What happens next?
Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)
If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.
If you need more immediate feedback you can try reaching out on Discord in the Community Platform
development
channel.