-
Notifications
You must be signed in to change notification settings - Fork 237
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 Kit specific patterns to be used with the unbranded template #842
Conversation
86d7357
to
87ad4b7
Compare
This ensures the extensions and other aspects of the main layout, work as expected.
Ensures all the same features are avaliable, for example tasks lists and step by steps.
87ad4b7
to
541e232
Compare
@import "node_modules/govuk-frontend/govuk/settings/colours-applied"; | ||
// Import settings first so we can override them before importing all of GOV.UK Frontend | ||
// If you need to enable compatibility mode or the legacy palette, do that *before* this import. | ||
@import "node_modules/govuk-frontend/govuk/settings/all"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since settings outputs no CSS it's safe to import the simpler 'all' entrypoint
|
||
{% block headIcons %} | ||
<link rel="shortcut icon" href="{{ asset_path }}images/unbranded.ico?0.18.3" type="image/x-icon" /> | ||
<link rel="mask-icon" href="{{ asset_path }}images/gov.uk_logotype_crown.svg?0.18.3" color="#0b0c0c"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rid of most of these icons since they're not needed and don't work anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icons are added when using layout.html
as they are used by GOV.UK Design System template. Could we use {{ super() }}
to add them here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Should the same changes be applied to lib/v6/govuk_template_unbranded
?
One thing is that this is quite a large breaking change. I wonder if there's any way we could do this without so many changes to the /app
folder? This is probably something you've already considered but I thought I'd mention it anyway.
@import "node_modules/govuk-frontend/govuk/settings/colours-palette"; | ||
@import "node_modules/govuk-frontend/govuk/settings/colours-applied"; | ||
// Import settings first so we can override them before importing all of GOV.UK Frontend | ||
// If you need to enable compatibility mode or the legacy palette, do that *before* this import. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking but it would be good to link to https://github.com/alphagov/govuk-frontend/blob/master/docs/installation/compatibility.md
|
||
{% block headIcons %} | ||
<link rel="shortcut icon" href="{{ asset_path }}images/unbranded.ico?0.18.3" type="image/x-icon" /> | ||
<link rel="mask-icon" href="{{ asset_path }}images/gov.uk_logotype_crown.svg?0.18.3" color="#0b0c0c"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icons are added when using layout.html
as they are used by GOV.UK Design System template. Could we use {{ super() }}
to add them here too?
|
||
{% block headIcons %} | ||
<link rel="shortcut icon" href="{{ asset_path }}images/unbranded.ico?0.18.3" type="image/x-icon" /> | ||
<link rel="mask-icon" href="{{ asset_path }}images/gov.uk_logotype_crown.svg?0.18.3" color="#0b0c0c"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The icons are still added when using layout.html
as they are used by GOV.UK Design System template. Could we use {{ super() }}
to add them here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the unbranded template we want to intentionally avoid the default icons as they're branded, so I don't think we want to use super here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that makes sense 👍
I think... we should keep that the same for compatibility reasons, but could be wrong.
What do you think this would break for users? |
Happy to pair on this, the v6 stuff can be a bit weird.
Okay the word "breaking" was a bit misleading, I meant it in the sense that users have to manually do something. When there are changes to |
I don't think V6 compatibility should expect to have features available in the current release, seems weird to me to update V6 so it works differently to how it used to work.
I think if users don't make these changes when they update it wont break anything with their existing code, they just wont get the fixes for the unbranded stuff? |
Spoke with Hanna and we need to make sure the CHANGELOG explains how to upgrade |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the IRL @NickColley 👍 Yeah CHANGELOG should have step-by-step instructions for updating /app
with the least number of changes needed.
Re: the v6 stuff, we're not going to change the template there as we don't have a lot of information about how people use it so we could end up breaking someone's template. Users should ideally be looking to move to the new version of the kit to get the latest improvements.
@@ -2,6 +2,20 @@ | |||
|
|||
## Fixes | |||
|
|||
### Allow Kit specific patterns to be used with the unbranded template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hannalaakso if you get time could you check these steps for factual accuracy please.
I've assigned @m-green to review this when he has time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @NickColley 👍 I'm not sure we need to mention step 3 as they'll get this change if they follow the kit update instructions.
Assigned this blocked as will need a review from Mark. |
c137234
to
34eafb4
Compare
Updates the unbranded template to extend the main layout, which allows users to use Prototype Kit specific patterns such as the task list and step-by-step patterns.
Closes #841