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

Add spacing to cookie banner confirmation message #1936

Merged
merged 1 commit into from
Feb 19, 2021
Merged

Conversation

maxgds
Copy link
Contributor

@maxgds maxgds commented Feb 17, 2021

What

Add spacing to cookie banner confirmation message

Why

It currently sits too close to the top of the browser

Visual Changes

Before

Screenshot 2021-02-17 at 11 29 16

After

Screenshot 2021-02-17 at 11 28 53

Screenshot 2021-02-17 at 11 35 28

@bevanloon bevanloon temporarily deployed to govuk-publis-banner-pad-i61lv8 February 17, 2021 11:31 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-banner-pad-i61lv8 February 17, 2021 11:33 Inactive
@maxgds maxgds marked this pull request as ready for review February 17, 2021 14:24
@@ -39,6 +39,7 @@ $govuk-cookie-banner-background: govuk-colour("light-grey", "grey-4");

.gem-c-cookie-banner__confirmation-message {
margin-right: govuk-spacing(4);
padding-top: govuk-spacing(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to have the padding on the overall component parent rather than add specific padding to child elements that does the same thing - although it looks like the problem here is the DS styles rather than yours.

I would remove the top padding from govuk-cookie-banner and put it on gem-c-cookie-banner instead, which would pad that and the confirmation message as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's messy with the required overrides, but I accept your point about applying it to the parent consistently - have updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that seems better to me. Unfortunately the spacing isn't consistent, due to a child of the confirmation message having padding of its own...

Screenshot 2021-02-17 at 17 08 12

...but to be honest at this point if these are all DS styles then I think you've already done enough to try to make this consistent and any further discussion should happen at a different level, so I'm going to approve since you've already solved the problem you set out to solve.

@bevanloon bevanloon temporarily deployed to govuk-publis-banner-pad-i61lv8 February 17, 2021 15:40 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-banner-pad-i61lv8 February 17, 2021 15:43 Inactive
@bevanloon bevanloon temporarily deployed to govuk-publis-banner-pad-i61lv8 February 19, 2021 10:18 Inactive
@chris-gds chris-gds merged commit 220193a into master Feb 19, 2021
@chris-gds chris-gds deleted the banner-padding branch February 19, 2021 10:21
This was referenced Mar 11, 2021
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.

4 participants