Skip to content
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

VACMS 15943: Move promo banner into header tag #1798

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

chriskim2311
Copy link
Contributor

@chriskim2311 chriskim2311 commented Nov 17, 2023

Summary

Moves promo banner under the header. Checked and this will not impact injected header.

Related issue(s)

department-of-veterans-affairs/va.gov-cms#15943

Testing done

Tested the header markup to make sure the promo banner element is inside the header tag

What areas of the site does it impact?

Header

Acceptance criteria

  • The <va-promo-banner> is wrapped within the <header>
  • The injected header is not impacted
  • Requires design review
  • Requires accessibility review

@chriskim2311 chriskim2311 marked this pull request as ready for review November 17, 2023 19:03
@chriskim2311 chriskim2311 requested review from a team as code owners November 17, 2023 19:03
@laflannery
Copy link

@chriskim2311 This is exactly what I wanted, and I just want to note that this is going to create 3 new axe errors (1, 2 & 4 in this screenshot). These will be resolved as soon as this DST ticket is released though:
image

@thejordanwood
Copy link

This looks good to me!

@chriskim2311
Copy link
Contributor Author

Thanks for the quick reviews! Will merge once I get a code review

Copy link
Contributor

@randimays randimays left a comment

Choose a reason for hiding this comment

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

LGTM

@chriskim2311 chriskim2311 merged commit 437a0eb into main Nov 20, 2023
24 checks passed
@chriskim2311 chriskim2311 deleted the 15943-va-promo-banner-a11y-fix branch November 20, 2023 17:51
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.

4 participants