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

Implement the Tudor crown in the Header component (v5.x) #4297

Merged
merged 3 commits into from
Jan 11, 2024

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Oct 3, 2023

Implements the Tudor crown change and new GOV.UK logo lockup in the Header component. Part of #4178.

The bulk of changes originally in this PR have now been merged in other PRs:

See #4278 for the equivalent changes for Frontend v4 and earlier.

Warning

This PR should not be merged until we have approval to do so.

Changes

  • Adds the new GOV.UK logo lockup (with Tudor crown) to the Header component, behind a useTudorCrown parameter, similar to the v3 and v4 releases.
  • Adds parameter to Nunjucks documentation.
  • Adds a new test for the parameter.
  • Updates changelog.

@querkmachine querkmachine self-assigned this Oct 3, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4297 October 3, 2023 16:14 Inactive
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 112.45 KiB
dist/govuk-frontend-development.min.js 38.58 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 78.74 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 73.99 KiB
packages/govuk-frontend/dist/govuk/all.mjs 3.86 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 112.44 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 38.57 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.38 KiB

Modules

File Size
all.mjs 70.32 KiB
components/accordion/accordion.mjs 21.67 KiB
components/button/button.mjs 4.7 KiB
components/character-count/character-count.mjs 21.24 KiB
components/checkboxes/checkboxes.mjs 5.83 KiB
components/error-summary/error-summary.mjs 6.57 KiB
components/exit-this-page/exit-this-page.mjs 16.08 KiB
components/header/header.mjs 4.46 KiB
components/notification-banner/notification-banner.mjs 4.93 KiB
components/radios/radios.mjs 4.83 KiB
components/skip-link/skip-link.mjs 4.39 KiB
components/tabs/tabs.mjs 10.16 KiB

View stats and visualisations on the review app


Action run for 66563cd

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4297 October 3, 2023 16:48 Inactive
@36degrees
Copy link
Contributor

Will also close #1739.

@36degrees 36degrees linked an issue Oct 4, 2023 that may be closed by this pull request
8 tasks
@querkmachine querkmachine requested a review from a team October 6, 2023 07:56
@querkmachine querkmachine changed the title Implement the Tudor crown in the Header component (v5.x) [DO NOT MERGE] Implement the Tudor crown in the Header component (v5.x) Oct 19, 2023
@querkmachine querkmachine marked this pull request as ready for review October 19, 2023 08:56
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4297 October 19, 2023 17:17 Inactive
@querkmachine querkmachine linked an issue Oct 30, 2023 that may be closed by this pull request
Comment on lines 156 to 208
// These tests use a select that specifically looks for a <title> within the <head> of the page
// to prevent them from matching <title> elements within embedded SVGs.
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Looking ace @querkmachine

You know this bit about the 2px offset?

  • The top offset has been increased to 2px as this appears to better match the text baseline when the logo is paired with a product name.

It's fixed Chrome, but Safari is out by 1px still on @1x, @1.5x and @2x displays

Weirdly for in-betweens like 4K → 3K both Safari and Chrome are misaligned

Product name baseline

It's going to be quite noticeable as a vertical text shift between services:

Crown.logo.loop.mov

@querkmachine
Copy link
Member Author

@colinrotherham I'm not too concerned about misalignment between old crown and new. From the sounds of it there's gonna be a push for a rapid, widescale roll out of the new crown, so the two will ideally only coexist for a short time.

I'm not totally sure what to do about differences between browsers. The current offset seems to work for Chromium, Firefox and IE 11, and is only one pixel off in Safari, so barring implementing some Safari-specific hack I'd be inclined to say it's a 'good enough' fit.

For the tweener screen resolutions it likely just comes down to how a user agent tries to round subpixels to device pixels. I doubt there's too much we can really do to influence that.

@colinrotherham
Copy link
Contributor

colinrotherham commented Oct 30, 2023

@querkmachine Regarding the video, we're happy with a minor horizontal text jump but keep the same baseline?

E.g. Could you restore the logo's top: -2px top: -1px and adjust the product name instead?

Regarding Safari, the product name's 24px height and lack of vertical-align causes the main difference

Bit of a hammer, but try this and Chrome and Safari are pixel perfect again

.govuk-header__product-name {
+  position: relative.
+  top: 4px;
+  height: 30px;
-  line-height: 1;
+  line-height: 30px;
+  vertical-align: top;
}

@owenatgov
Copy link
Contributor

Weighing in on @colinrotherham's comment above: I'll leave it up to @querkmachine, I'm personally agnostic. I don't think the impact of the problem is big enough that it'll negatively impact any users or even be noticeable in a way that disrupts a user or takes them out of a journey.

If you wanted a decision, let's give Colin's suggestion a try. I did some very light testing of it and it seems to do the trick. I'm a little worried about the suggested line-height override as this is currently being powered by govuk-typography-responsive. I haven't tried passing it a px value as an override so it's worth checking that that's not going to create a mess.

@owenatgov
Copy link
Contributor

Also: once that comment is resolved I'm personally happy with this PR 💪🏻 I won't step on Colin's reviewer toes anymore though.

@colinrotherham
Copy link
Contributor

colinrotherham commented Nov 3, 2023

@owenatgov @querkmachine I'm a Safari user so my legacy will be seeing the 1px alignment issue forever more 😆

I'm happy to help with a fix? The suggestion was a very heavy hammer so needs refining

@colinrotherham
Copy link
Contributor

@owenatgov @querkmachine I've had a go in #4423

@querkmachine
Copy link
Member Author

Popping this back into draft for now as it will require some changes to work alongside the transitional logo that is likely to go out with 5.0—namely adding a feature flag to switch logo.

I have been wondering if this one actually needs a feature flag. In the v3/v4 branches, changing the crown SVG also meant changing the fallback PNG and some of the CSS. Here, those surrounding changes will have happened in 5.0, so all 5.x has to do is replace the embedded SVG, and I'm not sure we technically need a flag to pull that off.

querkmachine added a commit that referenced this pull request Jan 8, 2024
Includes changelog notes for both #4297 and #4354, as they are presented as a single entry within the changelog
querkmachine added a commit that referenced this pull request Jan 8, 2024
Includes changelog notes for both #4297 and #4354, as they are presented as a single entry within the changelog
querkmachine added a commit that referenced this pull request Jan 8, 2024
Includes changelog notes for both #4297 and #4354, as they are presented as a single entry within the changelog
@querkmachine
Copy link
Member Author

querkmachine commented Jan 8, 2024

Sticking this back in review as I've made a few changes to it. Notably, I somehow blanked on adding the Nunjucks flag functionality mentioned in #4297 (comment). That is present now.

I've added a changelog to this PR intending to cover all of the crown changes included in 5.1, not just the ones in this PR. It's structurally similar to the v3/v4 changelog but with some different bits of code.

@colinrotherham
Copy link
Contributor

colinrotherham commented Jan 8, 2024

Thanks @querkmachine

Do we definitely consider it a breaking change to switch to the correct crown?

Would we want useTudorCrown: false to "opt out" rather than useTudorCrown: true to "opt in"?

@querkmachine
Copy link
Member Author

@colinrotherham I'm still somewhat undecided on whether it is, personally, however I'd mentioned the existence of a feature flag in update documentation shared with the cross-team crown change group and consistency with those expectations is probably more valuable.

Similarly, it allows the update instructions for 5.1, 4.8 and 3.15 to be essentially identical to one another, hopefully simplifying comms around the crown change.

Agree that it should be opt-out going forward. To my mind that would mean having it be opt-in for 5.1 as a 'transition period' and then change it be opt-out from 5.2 onward, deprecating the flag in the process. Does that sound sensible?

CHANGELOG.md Outdated Show resolved Hide resolved
@colinrotherham colinrotherham self-requested a review January 8, 2024 13:17
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

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

Makes sense @querkmachine

If we choose not to have a transitional period, the default can be flipped in another PR so it's not a blocker 🙌

Includes changelog notes for both #4297 and #4354, as they are presented as a single entry within the changelog
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-4297 January 11, 2024 08:28 Inactive
@querkmachine querkmachine merged commit 53b9e25 into main Jan 11, 2024
45 checks passed
@querkmachine querkmachine deleted the new-crown-v5 branch January 11, 2024 09:03
@querkmachine querkmachine mentioned this pull request Feb 5, 2024
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.

Implement the new crown assets in Frontend (v5.x) Use SVG lockup for GOV.UK in header
5 participants