-
Notifications
You must be signed in to change notification settings - Fork 334
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
Implement transitional crown in the Header component (v5.0) #4449
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for e346a4a |
packages/govuk-frontend/src/govuk/components/header/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/header/template.njk
Outdated
Show resolved
Hide resolved
Thanks @querkmachine Would you mind keeping the commit 058b443 separate for history sake? Extra information in the commit message Did you want to slightly smaller (file size) logo from commit 8e0ce07 too? |
d52cf59
to
6b35bfd
Compare
😎 |
packages/govuk-frontend/src/govuk/components/header/template.njk
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/header/template.njk
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/header/template.njk
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/header/template.test.js
Outdated
Show resolved
Hide resolved
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.
Think this is looking good @querkmachine
Added a few minor comments then will want a CHANGELOG entry and we're spot on
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.
Nice work with all this @querkmachine
Would you mind giving it a rebase with main
? This PR and #4445 will need merging in order as both have ### Breaking changes
but think we're happy to get them in the next v5 beta
@claireashworth is keeping a Google Doc up to date with a re-ordered CHANGELOG so I'll flag this change too |
- Adds logo lockup of St Edward's crown with integrated GOV.UK text - Removes HTML and classes that used to wrap the logo group as these are no longer needed - Adjust the vertical offset of the logo to align with productName text - Tweak attributes to ensure logo is available to assistive technologies
Some minor defects snuck into the previous lockup that was used, potentially introduced due to rounding during compression. These were not visible at typical sizes, but may have affected how browsers rounded subpixel values. - There was a 'notch' inside of the G caused by two closely located points being rounded in opposite directions. - The . character was slightly deformed and not a clean circle.
- Snap points to whole pixel values where very close (e.g. 27.005px to 27px) - Fix positioning relative to product name Co-authored-by: Colin Rotherham <work@colinr.com>
The link was given `display: inline` at tablet sizes but the product name doesn’t wrap until desktop: #2549
38ef31b
to
e346a4a
Compare
Implements transitional crown in Frontend 5.0. That is, an asset that utilises the new lockup agreed for Frontend v5 (#1739), but using the St. Edward's crown. Part of #4440.
Changes
role="img"
and an embedded<title>
element. This combination appears to work consistently across screen readers in Deque's assessment of SVG implementations ("SVG pattern 5") and does not require the use of globally uniqueid
s.fill="currentcolor"
definition has been removed as the same style is being applied by CSS..govuk-header__logotype
span that wrapped both logo elements has been removed..govuk-header__logotype-crown
class has been renamed.govuk-header__logotype
..govuk-header__logotype-text
span has been removed.<title>
element so that they don't conflict with the SVG's<title>
element.