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

Allow border and hover colours to be overridden #4268

Merged
merged 2 commits into from
Oct 4, 2023
Merged

Allow border and hover colours to be overridden #4268

merged 2 commits into from
Oct 4, 2023

Conversation

paulrobertlloyd
Copy link
Contributor

@paulrobertlloyd paulrobertlloyd commented Sep 28, 2023

$govuk-border-colour and $govuk-hover-colour are missing !default declarations, meaning its not possible to override these values in upstream projects as it is with other colour values in this file.

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks @paulrobertlloyd! Could you please add a changelog entry? This would just be a fix so only needs to follow that format and be a list item under the unreleased fixes heading. I'm also happy to do this for you.

@paulrobertlloyd
Copy link
Contributor Author

@owenatgov Done!

Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

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

Lovely. Just needs a 2nd review.

@owenatgov owenatgov added sass / css and removed awaiting triage Needs triaging by team labels Oct 2, 2023
@owenatgov owenatgov requested a review from a team October 2, 2023 09:21
Copy link
Member

@querkmachine querkmachine left a comment

Choose a reason for hiding this comment

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

LGTM! (Though it looks like a merge conflict snuck in literally as I was reviewing this.)

The name of the $govuk-hover-colour variable really doesn't helpfully describe what it actually does, huh? 😅

@paulrobertlloyd
Copy link
Contributor Author

Merge conflict resolved.

@colinrotherham
Copy link
Contributor

Cheers @paulrobertlloyd, do you mind rebasing or squashing away the merge commit that's snuck in?

@paulrobertlloyd
Copy link
Contributor Author

Isn't there a big green button on GitHub that lets you do that? Unless it's disabled in this repo? (I've been making these changes via the GitHub website, safe faffing about with the repo on my machine again!)

@colinrotherham
Copy link
Contributor

We have it turned off 😣

@paulrobertlloyd
Copy link
Contributor Author

Bugger. Faff it is then.

@paulrobertlloyd
Copy link
Contributor Author

Sorry, I’ve tried squashing commits in my patch-1 branch on my machine, but I’ve reached the limit of my Git skills (I briefly had this PR wanting to add over 500 commits). Somebody on your side will have to either do it, or tell me how. 😔

`$govuk-border-colour` and `$govuk-hover-colour` are missing a `!default` declaration, meaning its not possible to override these values in upstream projects.
@36degrees
Copy link
Contributor

I've rebased your branch against the latest main (which 'automatically' gets rid of the merge commit) and resolved a merge conflict with the changelog 👍🏻

@owenatgov
Copy link
Contributor

With 2 approvals and a clean commit history (thanks @36degrees!) I'm going to merge this. Thanks again @paulrobertlloyd!

@owenatgov owenatgov merged commit c3f4b2d into alphagov:main Oct 4, 2023
43 checks passed
@paulrobertlloyd
Copy link
Contributor Author

Thanks @36degrees!

@paulrobertlloyd paulrobertlloyd deleted the patch-1 branch October 4, 2023 09:42
@romaricpascal romaricpascal mentioned this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

5 participants