Skip to content

Add is_sectioning check to Card and Overview #1936

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.

Sign up for GitHub

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
Merged
merged 9 commits into from
Jul 12, 2022
Merged

Add is_sectioning check to Card and Overview #1936

merged 9 commits into from
Jul 12, 2022

Conversation

@spaceninja
Copy link
Member

@spaceninja spaceninja commented Jul 12, 2022 •
edited
Loading

Overview

This PR updates the template for the Card component and Overview object
to check if tag_name is a sectioning element like article or
section, and then change the default for header_tag_element and
footer_tag_element to either header/footer or div.

This avoids an issue when header elements are rendered in a div,
it causes useless "banner" landmarks to display in the VO Rotor.

Testing

Confirm tests pass on this PR.

Review the Card and Overview stories on the preview deploy. There should be no visual changes.

@spaceninja
Add is_sectioning check to Card and Overview
983ffef 
This PR updates the template for the Card component and Overview object
to check if `tag_name` is a sectioning element like `article` or
`section`, and then change the default for `header_tag_element` and
`footer_tag_element` to either `header`/`footer` or `div`.

This avoids an issue when `header` elements are rendered in a `div`,
it causes useless "banner" landmarks to display in the VO Rotor.
@changeset-bot
Copy link

changeset-bot bot commented Jul 12, 2022 •
edited
Loading

🦋 Changeset detected

Latest commit: 7b02ae2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@cloudfour/patterns Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Jul 12, 2022 •
edited
Loading

✅ Deploy Preview for cloudfour-patterns ready!

Name Link
🔨 Latest commit 7b02ae2
🔍 Latest deploy log https://app.netlify.com/sites/cloudfour-patterns/deploys/62ce0016ac3b730008b97b0d
😎 Deploy Preview https://deploy-preview-1936--cloudfour-patterns.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@spaceninja spaceninja marked this pull request as ready for review July 12, 2022 21:50
tylersticka
tylersticka reviewed Jul 12, 2022
View reviewed changes
gerardo-rodriguez
gerardo-rodriguez reviewed Jul 12, 2022
View reviewed changes
tylersticka
tylersticka previously approved these changes Jul 12, 2022
View reviewed changes
gerardo-rodriguez
gerardo-rodriguez reviewed Jul 12, 2022
View reviewed changes
gerardo-rodriguez
gerardo-rodriguez previously approved these changes Jul 12, 2022
View reviewed changes
Copy link
Member

@gerardo-rodriguez gerardo-rodriguez left a comment

Choose a reason for hiding this comment

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

Nice work, @spaceninja! 🎉

Paul-Hebert
Paul-Hebert previously approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Left a few nits inline.

Out of curiosity are there use cases where we're using Overview as something other than a section? Or are we just making this change to be safe?

@spaceninja spaceninja dismissed stale reviews from Paul-Hebert and gerardo-rodriguez via 0d0f3e4 July 12, 2022 23:08
spaceninja and others added 4 commits July 12, 2022 16:08
@spaceninja @Paul-Hebert
Update src/components/card/card.test.ts
0d0f3e4 
Co-authored-by: Paul Hebert <paul@cloudfour.com>
@spaceninja @Paul-Hebert
Update src/components/card/card.test.ts
1941aaf 
Co-authored-by: Paul Hebert <paul@cloudfour.com>
@spaceninja
add comments
d3edd76
@spaceninja
lint
7b02ae2
@spaceninja
Copy link
Member Author

spaceninja commented Jul 12, 2022

are there use cases where we're using Overview as something other than a section?

Yeah, it was part of the changes in https://github.com/cloudfour/cloudfour.com-wp/pull/778

@Paul-Hebert
Copy link
Contributor

Paul-Hebert commented Jul 12, 2022

are there use cases where we're using Overview as something other than a section?

Yeah, it was part of the changes in https://github.com/cloudfour/cloudfour.com-wp/pull/778

Cool! Carry on then 👍

Paul-Hebert
Paul-Hebert approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@Paul-Hebert Paul-Hebert left a comment

Choose a reason for hiding this comment

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

🚢

@spaceninja spaceninja merged commit 4708f00 into v-next Jul 12, 2022
@spaceninja spaceninja deleted the chore/sectioning-check branch July 12, 2022 23:19
@github-actions github-actions bot mentioned this pull request Jul 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gerardo-rodriguez gerardo-rodriguez gerardo-rodriguez approved these changes

@tylersticka tylersticka tylersticka left review comments

+1 more reviewer

@Paul-Hebert Paul-Hebert Paul-Hebert approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Do not use header or footer in Cards using div

5 participants

@spaceninja @Paul-Hebert @tylersticka @gerardo-rodriguez