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

RFC 2: Add spacing (margins) model to components #292

Closed
wants to merge 1 commit into from

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented May 1, 2018

@vanitabarrett
Copy link
Contributor

I'm not sure about the using a "value" to set the margins - the 1-5 system seems fairly arbitrary and is confusing to understand. I'd find it confusing to supply '3' to get a half margin, for example.

Along the same lines, I'm also not sure on the readability of margins: [1, 0, 2, 0]. Someone new to GOV.UK/our frontend wouldn't be able to understand that without cross-referencing with some docs somewhere.

A couple of questions, more than comments:

  • What happens if someone supplies margins: [2]? How do we interpret this? Do we follow CSS patterns or only process the margin values we have? (so margin: 2, 2, 2, 2 vs margin: 2, 0, 0, 0)
  • Will this pattern work for components which, for some reason, have unusual spacing needs? E.g: values outside our spacing grid or negative margins. Or do we just not allow this?

@maxgds
Copy link
Contributor

maxgds commented May 1, 2018

I agree with Vanita on the arbitrary values mapping to two-thirds etc - I saw that you'd raised it as a possible concern already, but I do think it's somewhat opaque in meaning and there would be a fair bit of digging around to find the relevant mapping when building or implementing a new component. It would be good to tidy up the differences between components (false vs 0 vs "" for flags, for example) but I think it needs to be more understandable at a glance than this proposition.

@andysellick
Copy link
Contributor Author

I agree with all of these points. Does anyone have any suggestions?

@andysellick
Copy link
Contributor Author

I think any implementation we come up is likely to be complicated enough that it will need to be documented for developers to be able to add it to any new or existing components.

I guess an alternative is to take the smallest value (say, gutter-one-quarter) and do the scale as multiples of that. So 1 would be one quarter, 2 would be two quarters, etc. It still doesn't match exactly but it makes a little bit more sense.

@tijmenb
Copy link
Contributor

tijmenb commented May 9, 2018

@NickColley
Copy link
Contributor

NickColley commented May 9, 2018

Awesome RFC, great to see something so well thought out. 👍


Alternatives to the proposed kind of scale include 't-shirt' sizing, but that can get confusing with 'xxxl' or 'xxxs' etc.

Similar problems would be ran into if you used 'large' 'larger' 'largerest' etc.

We've opted for a number based scale, with documentation explaining how it works.


In terms of the interface for the spacing, you could consider a more boring approach.

For the most part, vertical rhythm is best set via margin on the bottom of components.

With this in mind, the majority of times you'll only want to set margin-bottom.

So you could have margin_bottom: N where N is a number from your scale (and so on for the other sides).


Your questions:

we already have individual ways in which components handle margin, so this is just adding another one that may or may not be used by more than one component

This proposal could deprecate those methods, it may be easy to replace them if this RFC is accepted.

aligning the classes to a scale like this (1 to 5) is a little confusing as it doesn't correspond to the numbers involved

It'd be good to have you folks see the spacing scale in the design system, as it may help solve this problem.

this model only adds margin, it provides no option to have no margin i.e. it assumes no margin as the default. Components that already have margins by default may require more work to adapt to using this system.

Since most components can have the same default margin, we're setting the same margin by default.

This means for the most part you don't have to worry about spacing for vertical rhythm, and add spacing in for the cases where you need something different.

This is the same way the current components with spacing work, where they can have their spacing set to 0.


Finally, since the new GOV.UK Design System has a spacing scale that aims to solve this same issue, it would be good to align on this if possible.

I think it would provide a similar solution as described in this RFC.

@andysellick
Copy link
Contributor Author

Thanks for all the input everyone. I'm going to close this RFC for now. I think that while a system like this to apply margin to all components is possible, due to the complexity of the existing margins on components and how closely that's tied to the design of the site this issue is larger than just FE.

My recommendation is that we plan to take a look at this again later, perhaps as part of a mission team, and include a proper audit of the existing components and the design context in which they're used. My hope is that as GOV.UK becomes more componentised over time this will be easier to solve.

For now I'm going to try a model similar to the one proposed on one component (heading) and see how well it works.

@barrucadu barrucadu deleted the rfc-spacing branch January 2, 2019 15:42
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.

5 participants