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 department colours to components #296

Merged
merged 2 commits into from
May 4, 2018

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented May 3, 2018

This PR follows the discussion over this RFC to add department colour branding to components.

The main point of discussion in the RFC centred around using colour specific classes instead of department ones, to avoid duplication and make the classnames more meaningful. I have decided not to implement this for the following reasons:

  • coming up with classnames based on colours is hard i.e. colour--light-red, colour--lighter-red doesn't scale well if new colours are needed later, and neither does colour--red-1, colour--red-2 etc.
  • changing the backend so all the organisations could have this different approach to branding would have involved a lot of work that we don't have time for this mission

I've written the code to be as flexible as possible (classes are all output from a helper method) so if a new approach is needed in future it shouldn't be too difficult to adapt. This change therefore broadly follows the originally proposed idea, with some specific changes:

  • the mixin generates a single parent class containing child classes for font colour (using the websafe colour) and border colour (using the secondary colour)
  • branding classes have been improved to be more BEM-like

In total this adds around 4KB to the CSS in the gem.

This PR includes two commits; one to add the colours and one to show how the model works on a component (note that the link to documentation in it won't work yet).

Trello card: https://trello.com/c/r4PLGbH5/58-create-a-branding-model-for-components

@tijmenb tijmenb temporarily deployed to govuk-publishing-compon-pr-296 May 3, 2018 14:51 Inactive
@andysellick andysellick force-pushed the add-department-colours-to-components branch from 003c6ca to ec40611 Compare May 3, 2018 14:52
@tijmenb tijmenb temporarily deployed to govuk-publishing-compon-pr-296 May 3, 2018 14:52 Inactive
@andysellick andysellick force-pushed the add-department-colours-to-components branch from ec40611 to 2f4db8d Compare May 4, 2018 08:42
@tijmenb tijmenb temporarily deployed to govuk-publishing-compon-pr-296 May 4, 2018 08:42 Inactive
@andysellick andysellick changed the title [DO NOT MERGE] [WIP] Add department colours to components Add department colours to components May 4, 2018
}
}

@include organisation-brand-colour();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need the ()?

@vanitabarrett
Copy link
Contributor

I like this approach, I think it makes sense to use the logic in govuk_frontend_toolkit. Perhaps in the future we can revisit this and see how we can make it more specific to colour rather than department, but this seems a good solution for now to prevent blocking your work.

Happy to re-review and 👍 once you've added some documentation for the document list component using the department colours

@andysellick andysellick force-pushed the add-department-colours-to-components branch from 2f4db8d to 0fc5a79 Compare May 4, 2018 10:05
@tijmenb tijmenb temporarily deployed to govuk-publishing-compon-pr-296 May 4, 2018 10:05 Inactive
@andysellick andysellick force-pushed the add-department-colours-to-components branch from 0fc5a79 to aba82e4 Compare May 4, 2018 10:09
@tijmenb tijmenb temporarily deployed to govuk-publishing-compon-pr-296 May 4, 2018 10:09 Inactive
@andysellick andysellick force-pushed the add-department-colours-to-components branch from aba82e4 to db727b8 Compare May 4, 2018 10:57
@andysellick andysellick merged commit 3dd8505 into master May 4, 2018
@andysellick andysellick deleted the add-department-colours-to-components branch May 4, 2018 11:56
Copy link
Contributor

@tijmenb tijmenb left a comment

Choose a reason for hiding this comment

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

Sorry for the late comments, I missed this PR.

brand_helper = GovukPublishingComponents::Presenters::BrandHelper.new(brand)
%>

<div class="gem-c-component <%= brand_helper.get_brand %>">
Copy link
Contributor

Choose a reason for hiding this comment

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

We mostly avoid prefixing getters with get_ (I'm not sure why Rubocop hasn't picked that up, I'll investigate).

To me get_brand also implies that it'll return the brand passed in. Perhaps brand_class would reveal the intention more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestions, thanks. See this PR -> #300

%>

<div class="gem-c-component <%= brand_helper.get_brand %>">
<div class="gem-c-component__title <%= brand_helper.get_brand_element("border-color") %>">
Copy link
Contributor

@tijmenb tijmenb May 9, 2018

Choose a reason for hiding this comment

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

I think it's worth it creating separate method for this and color to avoid typos.

Perhaps something like:

- <%= brand_helper.get_brand_element("border-color") %>">
+ <%= brand_helper.border_color_class %>
- <%= brand_helper.get_brand_element("color") %>">
+ <%= brand_helper.color_class %>

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.

3 participants