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

Unneeded padding on warning component? #1083

Closed
NickColley opened this issue Dec 3, 2018 · 9 comments
Closed

Unneeded padding on warning component? #1083

NickColley opened this issue Dec 3, 2018 · 9 comments

Comments

@NickColley
Copy link
Contributor

NickColley commented Dec 3, 2018

Noticed that the govuk-warning-text__text element has padding left that includes an extra 15px to compensate for a negative margin left (see below, dotted outline added by me to show boundary of component). Seems like this could be removed and the padding decreased, but there must be a reason for it?

screen shot 2018-12-03 at 11 30 12

Originally posted by @andysellick in alphagov/govuk-design-system-backlog#71 (comment)

@NickColley NickColley changed the title Noticed that the govuk-warning-text__text element has padding left that includes an extra 15px to compensate for a negative margin left (see below, dotted outline added by me to show boundary of component). Seems like this could be removed and the padding decreased, but there must be a reason for it? Unneeded padding on warning component? Dec 3, 2018
@36degrees
Copy link
Member

For reference, it looks like this was copied from Elements.

@NickColley
Copy link
Contributor Author

@andysellick I believe this was done to allow for multiple lines:

screen shot 2018-12-03 at 13 26 38

@36degrees
Copy link
Member

Yep, but removing the negative margin and reducing the padding appears to be the same:

- margin-left: -$govuk-gutter-half;
- padding-left: 65px;
+ padding-left: 50px;

screen shot 2018-12-03 at 13 44 55

@NickColley
Copy link
Contributor Author

NickColley commented Dec 3, 2018

That's weird, I've asked Andy if he wants to do a PR for this... If he's busy I'll turn this into a good first timer issue.

@andysellick
Copy link
Contributor

I am busy, but I'd like to contribute (if only to experience the process) so I'll try to raise a PR soon (also it's not a huge change). Thanks!

@NickColley
Copy link
Contributor Author

@andysellick will leave this open for you when you get a second then.

@stevenaproctor
Copy link

@NickColley @36degrees Do you think the icon should be vertically aligned to the middle of the paragraph, like in the example, or the top or the paragraph?

@NickColley
Copy link
Contributor Author

NickColley commented Dec 3, 2018

@stevenaproctor the content used there is really extreme and silly, so not sure in practice if that part of the design should be changed.

If you think we can improve it though, feel free to open another issue with some more real world example content, and what you think it should look like and we can consider it.

@stevenaproctor
Copy link

@NickColley I just think it scans better if the icon is at the top of the paragraph. You would naturally scan left to right, not left, down, up, right...if you see what I mean.

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

No branches or pull requests

4 participants