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

105 update to latest govuk frontend version #117

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

sarahmwong
Copy link
Collaborator

@sarahmwong sarahmwong commented Dec 6, 2024

Overview of changes

Update to 5.7.1 frontend - adding @cjrace as a reviewer as you've had experience updating this before too!

Everything looks ok apart from value boxes - tested this in master and still looks odd! I've raised #116 for Andy to revisit as this looks like a separate issue.

Removed some old manual changes from the css_changes.md file as no longer relevant

PR Checklist

  • I have updated the documentation (if needed)
  • I have added or updated tests for these changes (if applicable)
  • I have tested these changes locally using devtools::check()

Reviewer instructions

Run devtools::load_all

Run run_example()

Check that all css changes look ok and functionality still works

@sarahmwong sarahmwong requested a review from cjrace December 6, 2024 12:08
@sarahmwong sarahmwong linked an issue Dec 6, 2024 that may be closed by this pull request
Copy link
Collaborator

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Not fully tested everything, but leaving the comments I have for now so they're not lost (as I'm not around tomorrow)

Spotted an issue with accordions (though not cross checked against main yet to see if it's pre-existing)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we use this anywhere? Looks like the footer crest, though seems odd we didn't already have a version of it? Do we need to point the footer function this this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes this replaces govuk-crest.png, still sat in the folder here weirdly! I'll double check when I'm back on my personal comp whether this image was removed in the latest zip file or not, and see if we can remove. Footer function points to govuk-footer__copyright-logo in the css which has the right svg file referenced so no need to update I think

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given we just copy this in now (and we are full rem instead of norem), would it make sense to change to leaving the name as the same as the one from the GOV.UK ZIP? I think then it's just a case of updating the attach function to import this sheet as a dependency?

@@ -1,8 +1,7 @@
@charset "UTF-8";

:root {
--govuk-frontend-version: "5.4.0";
font-size: 16px;
Copy link
Collaborator

@cjrace cjrace Dec 11, 2024

Choose a reason for hiding this comment

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

Was removing this font size a GDS or us change? I think a lot of the fonts all seem smaller now than they were?

Screenshot from this branch (100% zoom in Chrome):
image

Screenshot from the master branch (100% zoom in Chrome):
image

Copy link
Collaborator

@cjrace cjrace left a comment

Choose a reason for hiding this comment

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

Have finished my review now, there's only one other odd thing I've noticed (may or may not be related to the fonts comment)

Select input (dropdown) now cuts the bottom of text off

Before
image

After
image

@sarahmwong
Copy link
Collaborator Author

Thanks for your comments @cjrace! I think these have been addressed now - think the font fix fixes the selection box (looks like this to me):image

Accordions issue is pre-existing in master good spot! I've raised #123 for a fix - can ask Jeni to take a look unless you wanted to have a go with faffing with the JS! 😄

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.

Update to latest GOV.UK frontend version
2 participants