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 outline to tag for customised colours #855

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

NickColley
Copy link
Contributor

@NickColley NickColley commented Jul 2, 2018

Adds a border to the tag element so it makes more sense when colours are overridden.

The screenshots I've included here show the tag component when used in the phase banner component.

Uses relative spacing to keep alignment when the label is on a smaller screen.

Before overriden colours

screen shot 2018-07-02 at 12 48 15

After overriden colours

screen shot 2018-07-02 at 12 48 10

Before regular colours

screen shot 2018-07-02 at 12 47 18

After regular colours

screen shot 2018-07-02 at 12 47 22

Smaller screens:

Before overriden colours

screen shot 2018-07-02 at 12 53 56

After overriden colours

screen shot 2018-07-02 at 12 53 52

Before regular colours

screen shot 2018-07-02 at 12 53 47

After regular colours

screen shot 2018-07-02 at 12 53 41

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-855 July 2, 2018 11:53 Inactive
@NickColley NickColley requested a review from dashouse July 2, 2018 11:55
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

I know you flagged this as part of your comment, but this results in the height of this component changing from 25px to 27px. As far as I know, all components should have a height which is a multiple of 5px.


display: inline-block;
padding: govuk-spacing(1) 8px 0;
padding-top: .3125em;
Copy link
Contributor

Choose a reason for hiding this comment

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

Given we've got #845 open to try and remove our only current usage of em for padding, do we want to be introducing it again here, or should we be using pixels?

Also, can this be a single padding rule as the values are all the same?

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'd prefer to use relative units since it means when the tag is resized the padding is in proportion. Will show screenshots, before and after PX.

The padding results are not all the same, but I'll see if I can reduce the repeitition (I personally do not like shorthand since it's like doing function(many, arguments, order, isimportant) vs function({ order: 'not important' }) etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think other components like the button keep the same dimensions when the font-size reduces (because the line-height is constant) so I'd expect this component to work in the same way… especially as we want to keep the /5 vertical rhythm.

Totally failed to notice that padding-bottom was different so… that's fine 👍

Copy link
Contributor Author

@NickColley NickColley Jul 2, 2018

Choose a reason for hiding this comment

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

OK, good to know, thanks Ollie. I still think the relative spacing looks more balanced but if it's to get the grid alignment that makes sense I guess...

(Should probably have this written down)

@NickColley NickColley force-pushed the tag-border-when-overriding-colours branch from 36706b6 to 5b563cc Compare July 4, 2018 10:33
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-855 July 4, 2018 10:33 Inactive
@NickColley
Copy link
Contributor Author

@36degrees I've adjusted this PR based on your comments and ended up with something very similar to the original:

tag-colours-change

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-855 July 4, 2018 10:45 Inactive
@NickColley NickColley added this to the v1.1.0 milestone Jul 4, 2018
@NickColley NickColley force-pushed the tag-border-when-overriding-colours branch from dd3e3e6 to c23ee45 Compare July 4, 2018 15:52
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-855 July 4, 2018 15:52 Inactive
@NickColley NickColley removed this from the v1.1.0 milestone Jul 4, 2018

display: inline-block;
padding: govuk-spacing(1) 8px 0;
padding: 4px 6px 1px;
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid having to change the line height, padding, etc, can we add a transparent outline instead? The only change then needed would be:

    outline: 1px solid transparent;
    outline-offset: -1px;

screen shot 2018-07-04 at 12 05 57bst

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do yeah, would prefer to use borders since we can 'see' them. Up to you.

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'll change this, it also means the modifiers get outlines for free without additional CSS :)

@NickColley NickColley force-pushed the tag-border-when-overriding-colours branch from c23ee45 to da8ed7d Compare July 9, 2018 12:12
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-review-pr-855 July 9, 2018 12:12 Inactive
NickColley and others added 2 commits July 9, 2018 13:13
When a user customises their colours often the background is removed,
by adding a outline we ensure that the tag component still keeps it's meaning.
@NickColley
Copy link
Contributor Author

OK, went with the transparent outline approach in the end.

Before outline

screen shot 2018-07-09 at 13 15 07

After outline

screen shot 2018-07-09 at 13 15 03

@NickColley NickColley changed the title Add border to tag for overriden colours Add outline to tag for customised colours Jul 9, 2018
@36degrees
Copy link
Contributor

👏

@NickColley NickColley merged commit 15b6af0 into master Jul 9, 2018
@NickColley NickColley deleted the tag-border-when-overriding-colours branch July 9, 2018 14:15
@36degrees 36degrees added this to the v1.1.0 milestone Jul 11, 2018
@NickColley NickColley mentioned this pull request Jul 13, 2018
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