-
Notifications
You must be signed in to change notification settings - Fork 20
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
RFC 1: Add government department colours to components #287
Conversation
c110795
to
9bede6b
Compare
9bede6b
to
b3bc143
Compare
b3bc143
to
278f48f
Compare
278f48f
to
91ebfd5
Compare
There's one thing that comes to my mind, we are using the name of the departments to set their colours, when/if a department changes their name we have to come back and rename the css rules to a new name. Question: wouldn't be better to give them the option to choose a colour that we already have rules for? E.g: Dropdown with green,brown, red, we would then have |
@Davidslv that's a good point, if we had generic colour classes not tied to department names that would be simpler, and avoid duplication (some of the current colours are duplicated already). We would need to change the mechanism by which the colours are delivered, which could be tricky - I think it's in use in more places than whitehall, so it would mean creating a new mechanism alongside the current one and having them running in parallel for a while, potentially. I'd be slightly concerned about the creation of tech debt there. |
@andysellick I agree with @Davidslv - I think it makes a lot of sense to be using generic colour variables rather than tying them in with department names. I'm not 100% sure of how this currently works and what this change would mean in terms of tech debt/how complicated it is to implement, but I think if it's a good solution then running 2 mechanisms in parallel for a while to get this working is a good trade-off, we just need to make sure we make a note of any tech debt to revisit. |
Btw, I just want to give some heads up that this has been kinda of hardcoded in the backend, there's a class for the Organisations Brand Colour which is based on their name... I hope we can try to take this entropy away if we can, please :) |
This is good feedback. Some brief thoughts.
In terms of the RFC, if this change broadly fits in with the rest of the plan, does anyone have any other comments on any other part of it? |
|
||
### In the component | ||
|
||
Pass component parameters for link_colour and border_color as required, as strings that match the pre-defined list e.g. 'department-for-work-and-pensions'. |
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.
Do we need to be specific about the intention of the passed in variables?
So instead of:
<%= render 'govuk_publishing_components/components/title', link_colour: 'department-for-education', border_color: 'department-for-education' %>
Do this:
<%= render 'govuk_publishing_components/components/title', brand: 'department-for-education' %>
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.
Yeah, that's simpler.
Could the designs of these pages be reviewed? Is there a user need for these link styles? Is there a way to avoid these odd pages while also retaining the department branding? |
@NickColley those questions are out of scope for this RFC. |
@tijmenb I don't think it's unreasonable to approach problems like this from a design perspective, worth at least discussing this with designers, rather than building complex systems that could be avoided. |
CSS in the gem looks like this: | ||
|
||
``` | ||
.color-link--department-for-work-and-pensions .color-link { |
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.
Could consider theme
or brand
, for the namespace, to be consistent with @tijmenb 's suggestion.
Thanks for all the feedback everyone. I've made a decision and have opened a new PR with the changes. I've not implemented all the suggestions made here: if you disagree with the approach I'm happy to discuss further but please have a specific suggestion as to how your proposal could be implemented as I've got a lot to do this mission. Thanks! |
👉 Rendered version