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

Additional Tag modifier classes for different colours #1711

Merged
merged 2 commits into from
Mar 5, 2020
Merged

Additional Tag modifier classes for different colours #1711

merged 2 commits into from
Mar 5, 2020

Conversation

adamsilver
Copy link
Contributor

@adamsilver adamsilver commented Jan 24, 2020

I spoke with @36degrees and then @timpaul about contributing additional Tag component styles via modifier classes. A lot of services (citizen-facing and internal) use the Tag component for different statuses.

Most of the time, different colours are used as an enhancement to spot those different states easily. Most of the designs I've seen put white text on coloured backgrounds. At DfE we did the same but they fail contrast requirements.

So we now uses these styles which are accessible and use the tints and shades of the GOV.UK colour palette.

More information here:
alphagov/govuk-design-system-backlog#62 (comment)

Note:

  1. I would usually look for meaningful names for modifier classes like ‘urgent’ but statuses are often unique to the system and so I used colours for the class names giving users flexibility.

  2. I still need to add guidance / examples to the design system repo. I'm not sure what the process is on that i.e. do I wait for this to be merged or something more elaborate?

Screenshot:

image

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1711 January 24, 2020 11:41 Inactive
src/govuk/components/tag/template.test.js Outdated Show resolved Hide resolved
@@ -20,6 +20,8 @@
text-decoration: none;
text-transform: uppercase;

white-space: nowrap;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unrelated to the commit description – if it's required, can we make this change in another commit that explains why it's being added, or possibly make it part of a separate PR?

@@ -41,4 +43,50 @@
.govuk-tag--inactive {
background-color: govuk-colour("dark-grey", $legacy: "grey-1");
}

.govuk-tag--grey {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider whether there's a need for inactive and grey, or whether they can be the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I considered deleting it, but I thought that would delay the contribution as I assumed it would be a breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could consider marking it as deprecated, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, and actually, it's not documented on the Tag component page. Is it documented publically elsewhere?

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've left it in, and maybe the deprecation can be a release note? Or do I need to do something in the code to mark it as deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's not been documented we could consider dropping it in 4.0.0

Copy link
Member

Choose a reason for hiding this comment

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

Created an issue for dropping it in 4.0.0: #1757

I'll chat to @m-green about whether we want to add a deprecation notice for govuk-tag--inactive somewhere, seeing that we don't document the class anywhere currently.

Copy link
Member

Choose a reason for hiding this comment

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

@adamsilver Would you be okay to add something like

Deprecated. We will be removing this class in a future release, use `.govuk-tag--grey` instead.

above the govuk-tag--inactive class in the scss file? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hannalaakso done.

src/govuk/components/tag/tag.yaml Show resolved Hide resolved
@NickColley
Copy link
Contributor

NickColley commented Jan 24, 2020

I think it's worth carefully considering the modifier names, if we name them based on their colour and decide the colours in the future need to change they may not make sense anymore or require a breaking change?

Good ol' bootstrap has this sort of thing with their 'badges' and they use broad semantic names which might be worth considering:
https://getbootstrap.com/docs/4.4/components/badge/#contextual-variations

@adamsilver
Copy link
Contributor Author

adamsilver commented Jan 24, 2020

I think it's worth carefully considering the modifier names, if we name them based on their colour and decide the colours in the future need to change they may not make sense anymore or require a breaking change?—@NickColley

Yeah—I did consider this as I said in the PR description.

I think it would be too difficult to come up with useful names (I checked Bootstrap) and I also think it could mean code within a service could be confusing...

For example, let's say we call the orange variant ‘warning-2’ and the yellow ‘warning-1’. (The reason for the numbers is because there aren't enough generic class names.)

Let's imagine I use ‘warning-1’ because in Apply for teaching training we want the ‘withdrawn’ status to be yellow. That would indicate “withdrawn’ is some sort of warning when it isn't.

Also, while colours have changed and may change again, I think it's less likely that colours will change to the point that an entire colour disappears. So perhaps the tint would change (like it did last time) but that sort of change was signficant to my knowledge and would affect multiple styles, components and patterns.

@NickColley that said, I think it's the design system team's call and if you think names are better, than would you be able to suggest the names for all of the colours?

@NickColley
Copy link
Contributor

@adamsilver I think you might be right that it's too difficult to have semantics that can anticipate what context these are used in and the colours dont change often.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1711 January 24, 2020 13:22 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1711 January 24, 2020 13:26 Inactive
@timpaul
Copy link
Contributor

timpaul commented Jan 27, 2020

@adamsilver I think you might be right that it's too difficult to have semantics that can anticipate what context these are used in and the colours dont change often.

Good discussion! I'm comfortable with the colour-based class names, as they're aligned with the names in our colour palette.

I totally agree that we can't anticipate all future semantic states that the tags will be used to represent. But we might converge on a few one day (eg. 'success', 'warning', 'fail' etc.) - does this approach block us from doing that in the future?

@adamsilver
Copy link
Contributor Author

adamsilver commented Jan 27, 2020

But we might converge on a few one day (eg. 'success', 'warning', 'fail' etc.) - does this approach block us from doing that in the future?—@timpaul

I think that would be fine because we could just add an additional govuk-tag--success class name as a minor release.

@peteryates
Copy link
Contributor

I don't know if this has been brought up elsewhere but, to me at least, the red and orange look very similar. To the point where I might not notice they were different. To a slightly lesser extent, the red and pink do too.

@adamsilver
Copy link
Contributor Author

@peteryates some of us at DfE thought the same thing, but we weren't overly concerned because:

  1. Colour is only used as an enhancement
  2. You don't have to use both colours if you don't want to
  3. They may not appear right next to each in the interface

@pripley-ea
Copy link

tag-colour-vision
I think these will be very useful and I like what you have done with matching the text colour with the background hue.

I've just run a quick test to see how a user with Protanopia would see them as shown above (red colour vision deficiency) and they are still usable as the text is clear but some of the colours cross over so the only thing is to be careful if multiple tags are used together.

Really nice addition to the design system :)

@hannalaakso hannalaakso modified the milestones: Next, v3.6.0 Mar 3, 2020
@hannalaakso hannalaakso mentioned this pull request Mar 4, 2020
2 tasks
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

This is looking great 🎉 I think the following two bits just needs to be resolved and then we can merge this.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1711 March 5, 2020 09:14 Inactive
@hannalaakso
Copy link
Member

hannalaakso commented Mar 5, 2020

@adamsilver Thanks for updating the PR.

We're now thinking that 1d79100 should be moved to its own PR as it could possibly be a breaking change as it could change your layout (esp on mobile) by forcing a longer tag to stay on its own line. We should talk about it separately. Apologies for the back and forth on this 🙁

If you were able to remove the commit from this PR (or I can do it if you prefer) we can get this one merged. Thank you!

@adamsilver
Copy link
Contributor Author

@hannalaakso thank you—I'd be delighted to take you up on your offer to do it for me pls :)

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-1711 March 5, 2020 16:16 Inactive
Copy link
Member

@hannalaakso hannalaakso left a comment

Choose a reason for hiding this comment

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

Great work, thank you @adamsilver 👏

@hannalaakso hannalaakso merged commit 83d86af into alphagov:master Mar 5, 2020
@hannalaakso
Copy link
Member

On 13 February 2020 the Design System working group approved the addition of different coloured variants of this component.

(Just mentioning this for reference as I didn't see it explicitly mentioned in the comments on this PR.)

@36degrees 36degrees mentioned this pull request Mar 6, 2020
lfdebrux added a commit that referenced this pull request Nov 3, 2021
Class govuk-tag--inactive was replaced by govuk-tag--grey and deprecated
in PR #1711. This commit replaces references to it in our tests,
examples, and fixtures.
lfdebrux added a commit that referenced this pull request Nov 3, 2021
Class govuk-tag--inactive was replaced by govuk-tag--grey and deprecated
in PR #1711.

This commit removes the class completely. You should replace any uses of
this class with `.govuk-tag--grey`.
lfdebrux added a commit that referenced this pull request Nov 3, 2021
Class govuk-tag--inactive was replaced by govuk-tag--grey and deprecated
in PR #1711.

This commit removes the class completely. Users should replace any uses
of this class with `.govuk-tag--grey`.
lfdebrux added a commit that referenced this pull request Nov 4, 2021
Class govuk-tag--inactive was replaced by govuk-tag--grey and deprecated
in PR #1711.

This commit removes the class completely. Users should replace any uses
of this class with `.govuk-tag--grey`.

Co-authored-by: Oliver Byford <[email protected]>
lfdebrux added a commit that referenced this pull request Nov 4, 2021
Class `govuk-tag--inactive` was replaced by `govuk-tag--grey` and
deprecated in PR #1711. This commit replaces references to it in our
tests, examples, and fixtures.
lfdebrux added a commit that referenced this pull request Nov 4, 2021
Class govuk-tag--inactive was replaced by govuk-tag--grey and deprecated
in PR #1711.

This commit removes the class completely. Users should replace any uses
of this class with `.govuk-tag--grey`.

Co-authored-by: Oliver Byford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

8 participants