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

Tech debt/rationalise colour names #1863

Closed
wants to merge 46 commits into from

Conversation

marianayovcheva
Copy link
Contributor

@marianayovcheva marianayovcheva commented Mar 23, 2022

Originated from Issue #1418

  • Renamed the colours using the following structure: ${component}__{element}__{property}--{state}

  • Added a simple cads-colours functions that allows for legacy compatibility, useful for cases where colours of the design-system components are overriden by the product. Eg: advisernet colour-language.scss. Similar approach is used by govuk (Thanks to David R for the reference)

  • Kept the header and footer colour variables but moved them directly in the components as they are local variables, not global. Inspired by govuk

  • Updated the Storybook colour section with the new colour names

  • Tested the colour updates with the current state of the public-website (linked my local repo in package.json) - things looked good and VRT didn't return errors.
    I wanted to get your opinion on the colour names first and then proceed with the following:

  • Update the Changelog

  • Add a file that contains the old colour names mapped to the new ones (this will ensure we avoid the breaking change)

  • Add information for the users about how to rename the colours using the new naming conventions

@davidrapson
Copy link
Contributor

davidrapson commented Mar 24, 2022

Couple of thoughts here:

Backwards compatibility:

This is likely a pretty big breaking change as projects use variable names directly. It's worth doing but we might want to look at a backwards compatible way of changing things, so we can still keep aliases of the old names around.

Naming conventions:

I wonder if we should use a simpler naming convention here rather than a BEM inspired one. From the Sass docs:

💡 Fun fact:
Sass variables, like all Sass identifiers, treat hyphens and underscores as identical. This means that $font-size and $font_size both refer to the same variable. This is a historical holdover from the very early days of Sass, when it only allowed underscores in identifier names. Once Sass added support for hyphens to match CSS’s syntax, the two were made equivalent to make migration easier.

https://sass-lang.com/documentation/variables

This means that the following are all equivalent:

$cads-language__background-colour--hover
$cads_language--background-colour__hover
$cads_language_-background-colour-_hover
$cads_language__background-colour__hover
$cads-language--background-colour--hover

Based on this I wonder if a simple hyphen based naming convention would make sense? e.g. $cads-language-background-colour-hover. Would minimise the number of possible variations.

@davidrapson
Copy link
Contributor

Just realised this was still draft 🙈 carry on.

@marianayovcheva marianayovcheva linked an issue Mar 24, 2022 that may be closed by this pull request
$cads-language__brand--secondary: palette.$cads-palette__heritage-yellow !default;
$cads-language__brand--primary-contrast: palette.$cads-palette__white !default;
$cads-language__brand--adviser: palette.$cads-palette__blue-adviser !default;
$cads-language__brand--focus: palette.$cads-palette__yellow !default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I want to call it brand--focus because I don't know if fits brand naming. However, naming the colour just $cads-language__focus also doesn't feel quite right as focus is not an element. Ideas are welcome

<Description>{colourLanguageTable([['Adviser', 'brand-adviser']])}</Description>
<Description>
{colourLanguageTable([['Adviser', 'brand--adviser']])}
</Description>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm tempted to add $cads-language__error__colour and $cads-language__success__colour in the documentation but I'm not sure if this needs to be agreed with designers first.

@marianayovcheva marianayovcheva marked this pull request as ready for review March 24, 2022 14:20
@marianayovcheva
Copy link
Contributor Author

@davidrapson Tests are now working so PR is undrafted 🎉

  • backwards compatibility - you're absolutely right. I've added a bit more information in the description about next steps for this PR
  • naming conventions - I see your point, tbh I wouldn't normally bemify variable names. In my opinion thought this can be a good case for using BEM as it makes the variables easier to read and follow. It also makes it easier to keep colours consistent in the future - if a dev needs to add a new colour, they would need to follow the naming convention. Happy to have a further chat

@davidrapson
Copy link
Contributor

naming conventions - I see your point, tbh I wouldn't normally bemify variable names. In my opinion thought this can be a good case for using BEM as it makes the variables easier to read and follow. It also makes it easier to keep colours consistent in the future - if a dev needs to add a new colour, they would need to follow the naming convention. Happy to have a further chat

Yeah, we also technically use a BEM-ish style now. So maybe the difference I mentioned isn't a big deal? I think as long as we have a consistent pattern that's the most important thing. Another route might be to do something with maps (see https://github.com/biglotteryfund/blf-alpha/blob/master/assets/sass/_vars.scss#L70-L104) and write a helper function to access colours (https://github.com/biglotteryfund/blf-alpha/blob/master/assets/sass/_functions.scss#L12-L14)

Using a wrapping function might help with backwards compatibility as this is how GDS do things too https://github.com/alphagov/govuk-frontend/blob/main/src/govuk/settings/_colours-applied.scss#L33

color: $cads-footer__text-colour;
color: $cads-language__text__colour;
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably does need to be specific to the footer. By using the shared $cads-language__text__colour I'm not sure how we'd handle theming like with AdviserNet/Intranet as this colour needs to be reversed out in those cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, good shout! I think if we keep them, we can perhaps put the header and footer vars into different files - that was the bit that made me remove them in first place

@marianayovcheva
Copy link
Contributor Author

@davidrapson those links are super useful, thank you! I'll have a further think about how to structure the colours, I'm beginning to lean towards the blf approach as it seems quite clean to me. I'll also think about how to simplify the header/footer colours without wiping the theming functionality. For now I'll move the PR back to draft but will let you know when I reopen it.

@marianayovcheva marianayovcheva marked this pull request as draft March 25, 2022 09:50
@github-actions
Copy link

🚀 Netlify deployed ca-design-system as draft

https://6242ebf86b0e475cec39db8e--ca-design-system.netlify.app

@github-actions
Copy link

🚀 Netlify deployed ca-design-system as draft

https://6242ee96e235c863306301b6--ca-design-system.netlify.app

# Conflicts:
#	testing/visual-regression/backstop_data/bitmaps_reference/backstop_default_Foundations_Colours_0_document_0_Large.png
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

🚀 Netlify deployed ca-design-system as draft

https://624ff7f5f30f81503e95c298--ca-design-system.netlify.app

marianayovcheva and others added 13 commits August 19, 2022 11:16
# Conflicts:
#	demo/package-lock.json
#	scss/2-tools/_interactive-elements.scss
#	scss/6-components/_error-summary.scss
#	scss/6-components/_navigation.scss
#	scss/6-components/_section-links.scss
#	scss/6-components/forms/fields.scss
#	scss/6-components/forms/form-groups.scss
When styling links it's recommended to follow either Link Visited Hover
Focus Active (LVHFA) or Link Visited Focus Hover Active (LVFHA) when
applying focus states to avoid them cancelling each other out in
unexpected ways. This effect is most noticeable when theming.

LVHFA is the best fit for us as our focus styles are deliberately so
prominent.
marianayovcheva and others added 5 commits September 8, 2022 09:27
…lour-names-docs-theme

Rationalise colour names docs theme (amends / docs theme)
# Conflicts:
#	CHANGELOG.md
#	design-system-docs/frontend/styles/_settings.scss
#	scss/2-tools/_interactive-elements.scss
@davidrapson davidrapson reopened this Aug 21, 2023
@marianayovcheva
Copy link
Contributor Author

Closing this as the PR is very stale (pre Storybook removal). I'll raise a new PR with the updated files

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.

Rationalise colour var names
3 participants