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 support for theme-color #278

Merged
merged 2 commits into from
Feb 20, 2017
Merged

Conversation

willp-bl
Copy link
Contributor

@NickColley
Copy link
Contributor

Sweet thanks @willp-bl 👍

Can you show some screenshots of it in action?

@dsingleton
Copy link
Contributor

Are we happy using a hard coded version of a colour from our palette?

Looking at the template only it feels like a magic value. I think we'd want to include a comment at least, which shows where the value comes from, or ideally pull it in from the SCSS variable as part of the build process.

I don't want to slow down this PR too much, but we should consider how we'd keep this value in sync.

@willp-bl
Copy link
Contributor Author

I don't have access to BrowserStack, could someone lend a hand with the screenshots?

The hard coded colour is used a couple of lines above without a comment - https://github.com/alphagov/govuk_template/pull/278/files#diff-fe08901690b6fdc6f34614aaae900216R20

Happy to add a comment to explain to this PR but I don't know where it is from...

@willp-bl
Copy link
Contributor Author

Should have clicked that link, will add a comment

@dsingleton
Copy link
Contributor

@willp-bl Thanks! That feels much better. Could you use erb style comments? That will be useful if looking at the template source, but won't add to the page for end users.

<link rel="mask-icon" href="<%= asset_path 'gov.uk_logotype_crown.svg' %>" color="#0b0c0c">
<link rel="apple-touch-icon-precomposed" sizes="152x152" href="<%= asset_path "apple-touch-icon-152x152.png" %>">
<link rel="apple-touch-icon-precomposed" sizes="120x120" href="<%= asset_path "apple-touch-icon-120x120.png" %>">
<link rel="apple-touch-icon-precomposed" sizes="76x76" href="<%= asset_path "apple-touch-icon-76x76.png" %>">
<link rel="apple-touch-icon-precomposed" href="<%= asset_path "apple-touch-icon-60x60.png" %>">

<%# the colour used for mask-icon is the standard palette $black from
Copy link
Contributor

Choose a reason for hiding this comment

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

mask-icon should be theme-color here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just pushed a fix for that :)

@dsingleton
Copy link
Contributor

@willp-bl Thanks for changes! This is looking good to me.

I'm happy to merge unless anyone has objections?

@NickColley
Copy link
Contributor

I'm just going to check this out on an android device but after that 👍

@NickColley
Copy link
Contributor

Before:

image

After:

image

image

Looks even better without the cookie bar! Thanks Will 💯

@NickColley NickColley merged commit 052a7ae into alphagov:master Feb 20, 2017
@willp-bl willp-bl deleted the add_theme_colour branch February 20, 2017 13:21
robinwhittleton pushed a commit that referenced this pull request Mar 16, 2017
- Fix EJS template (PR #270)
- Add `theme-color` support to make the page surround in Chrome’s tab view on Android match the black GOV.UK header (PR #278)
- Add `text-decoration-skip: ink` to all links on GOV.UK (PR #281)
- Improve contrast of links when focused (PR #272)
- Make header text colour black when focused (PR #274)
@robinwhittleton robinwhittleton mentioned this pull request Mar 16, 2017
robinwhittleton pushed a commit that referenced this pull request Mar 29, 2017
- Fix EJS template (PR #270)
- Add `theme-color` support to make the page surround in Chrome’s tab view on Android match the black GOV.UK header (PR #278)
- Add `text-decoration-skip: ink` to all links on GOV.UK (PR #281)
- Improve contrast of links when focused (PR #272)
- Make header text colour black when focused (PR #274)
robinwhittleton pushed a commit that referenced this pull request Mar 29, 2017
- Fix EJS template (PR #270)
- Add `theme-color` support to make the page surround in Chrome’s tab view on Android match the black GOV.UK header (PR #278)
- Add `text-decoration-skip: ink` to all links on GOV.UK (PR #281)
- Improve contrast of links when focused (PR #272)
- Make header text colour black when focused (PR #274)
fofr pushed a commit to alphagov/static that referenced this pull request May 5, 2017
# 0.20.0

- Fix EJS template - PR #270 -
alphagov/govuk_template#270
- Add `theme-color` support to make the page surround in Chrome’s tab
view on Android match the black GOV.UK header - PR #278 -
alphagov/govuk_template#278
- Add `text-decoration-skip: ink` to all links on GOV.UK - PR #281 -
alphagov/govuk_template#281
- Improve contrast of links when focused - PR #272 -
alphagov/govuk_template#272
- Make header text colour black when focused - PR #274 -
alphagov/govuk_template#274

# 0.19.2

- Increase skiplink colour contrast - PR #263 -
alphagov/govuk_template#263
- Fix Scala compile issues for Play template - PR #261 -
alphagov/govuk_template#261

# 0.19.1

- Have focus outline appear outside of element rather than covering it
in Safari and Chrome - PR #259 -
alphagov/govuk_template#259
willpdp added a commit to willpdp/parliament.uk-pugin that referenced this pull request Apr 7, 2019
UsmanAfzal pushed a commit to ukparliament/parliament.uk-pugin that referenced this pull request Apr 8, 2019
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