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

Update to govuk-frontend 3.7.0 #75

Merged
merged 2 commits into from
Jun 30, 2020
Merged

Conversation

alex-ju
Copy link
Contributor

@alex-ju alex-ju commented Jun 19, 2020

This change is intended to be a temporary fix for a style leakage issue, caused by unsynchronized dependency on govuk-frontend (miller-columns-element in 3.6.0 and content-publisher in 3.7.0), by updating this repo to the latest version.

A proper fix should follow this PR by either scoping the govuk-frontend component styles (checkboxes, back-link, breadcrumbs) used in this repo or by making the miller-columns-element non-dependent on other components, leaving the required styles to only be imported in the project using it (e.g. content-publisher).

Back link in Content Publisher

BeforeAfter
Screenshot 2020-06-19 at 18 01 53 Screenshot 2020-06-19 at 18 04 10

@alex-ju alex-ju requested a review from kevindew June 19, 2020 17:10
@alex-ju alex-ju force-pushed the update-to-govuk-frontend-3-7 branch from 1be18f3 to db5cb51 Compare June 19, 2020 17:12
@alex-ju alex-ju requested a review from benthorner June 26, 2020 10:19
background: transparent;
text-decoration: underline;

Choose a reason for hiding this comment

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

Could you add some explanation for these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I've now added a description to this commit:

Updating to govuk-frontend requires updating the govuk-back-link class we apply to our button element in a similar way govuk-frontend applies it to an anchor element. More on this change on the original PR in govuk-frontend alphagov/govuk-frontend#1753 – more specifically https://github.com/alphagov/govuk-frontend/pull/1753/files#diff-4c6ef8a7bcb3d85bc2f7e7a0cd1fd50cR33

@@ -293,6 +293,12 @@
.govuk-list--number > li {
margin-bottom: 5px; } }

.govuk-list--spaced > li {

Choose a reason for hiding this comment

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

Could you add the command used here? (Or perhaps it should go in the README.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By running npm run build which is also triggered before the test suite is run.

Updating to govuk-frontend requires updating the govuk-back-link class we apply to our button element in a similar way govuk-frontend applies it to an anchor element. More on this change on the original PR in govuk-frontend alphagov/govuk-frontend#1753.
By running `npm run build`
@alex-ju alex-ju force-pushed the update-to-govuk-frontend-3-7 branch from db5cb51 to 4c6e533 Compare June 30, 2020 11:25
@benthorner
Copy link

Thanks for the updated commits @alex-ju. I can see that the style changes do reflect the upstream ones. What's now puzzling me - and what I should have asked in my first review (sorry) - is: what is the back link for?

I can see it gets put in here, but isn't displayed. So I'm wondering why we care about how it looks, if we can't see it 😆.

@alex-ju
Copy link
Contributor Author

alex-ju commented Jun 30, 2020

Thanks for the updated commits @alex-ju. I can see that the style changes do reflect the upstream ones. What's now puzzling me - and what I should have asked in my first review (sorry) - is: what is the back link for?

I can see it gets put in here, but isn't displayed. So I'm wondering why we care about how it looks, if we can't see it 😆.

Great question. The back link is used on mobile to allow upstream navigation across the taxonomy tree.

@benthorner
Copy link

Aha! Would have been helpful to have a before/after, but I've also learnt something, so no worries.

Now I can see it, are we going to fix the weird half chevron while we're here?

Screenshot 2020-06-30 at 16 03 18

@alex-ju
Copy link
Contributor Author

alex-ju commented Jun 30, 2020

Aha! Would have been helpful to have a before/after, but I've also learnt something, so no worries.

Now I can see it, are we going to fix the weird half chevron while we're here?

Screenshot 2020-06-30 at 16 03 18

I'm a bit confused by your comment 😕 there is a before and after in the pull request description and the whole point of this PR is to fix the chevron.

Copy link

@benthorner benthorner left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by your comment 😕 there is a before and after in the pull request description and the whole point of this PR is to fix the chevron.

Sorry, don't mind me 🤭

@alex-ju alex-ju merged commit 5f5f329 into master Jun 30, 2020
@alex-ju alex-ju deleted the update-to-govuk-frontend-3-7 branch June 30, 2020 15:20
alex-ju added a commit that referenced this pull request Jun 30, 2020
## 1.3.2

- Fix back-link styles by updating miller-columns-element to `govuk-frontend` 3.7 (PR #75)
@alex-ju alex-ju mentioned this pull request Jun 30, 2020
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.

2 participants