Vertical margin proposal #16
Conversation
proposals/vertical-rhythm.md
Outdated
| function rhythm(size, direction = 'bottom') { | ||
| try { | ||
| if (!['top', 'bottom'].includes(direction)) { | ||
| throw `direction must be either 'top' or 'bottom'`; |
There was a problem hiding this comment.
Probably want to throw an actual error here, as it has a stack.
There was a problem hiding this comment.
I've redone the error handling as a whole. Please can you check I've got the right idea?
proposals/vertical-rhythm.md
Outdated
|
|
||
| ```js | ||
| type Props = {| | ||
| variant: 'section' | 'semiSection', |
There was a problem hiding this comment.
Camel case or kebab case?
There was a problem hiding this comment.
👍 will go with kebab
proposals/vertical-rhythm.md
Outdated
| } | ||
| ``` | ||
|
|
||
| As for the `<Section />` component, it will take a boolean prop `isFirst` that will determine whether or not the margin should be added to top and bottom. |
There was a problem hiding this comment.
I thought we agreed you could assign both ways?:
<Section spacingTop="section" spacingBottom="semi-section" />I dont like the naming of spacingTop % spacingBottom so am open to suggestions?
There was a problem hiding this comment.
went with marginTop and marginBottom as that's explicit.
proposals/vertical-rhythm.md
Outdated
| The `spacing` module will be renamed and will allow an optional second argument to specify the direction of the margin. | ||
|
|
||
| ```js | ||
| function rhythm(size, direction = 'bottom') { |
There was a problem hiding this comment.
I think this name is worse than spacing tbh. One of the problems states that spacing is a confusing name because it could imply space in any direction, you've added that functionality here so surely the name makes sense now?
There was a problem hiding this comment.
or it can by renamed to [vSpacing | vspacing] - to make it more specific
There was a problem hiding this comment.
Its supposed to map to vertical rhythm rather than spacing. Maybe we should just name it that?
There was a problem hiding this comment.
gonna go with verticalMargin so there can be no confusion
RhythmI agree that the spacing util is very restrictive, and the name is confusing. There are certainly places in the UI on a micro level where it makes sense to be setting the margin on the top rather than the bottom, and this has been achieved in some places by duplicating the mixin's logic, which is not ideal. Enabling This proposal does have a shortcoming though in that it doesn't tackle setting padding. I personally think the CSS variables approach would still be the best way to resolve this fully. That way, on a component's styles you could set;
etc, without the need for a mixin, and the value of the variable would be dynamically updated in a root media query, so the individual components don't need to know about media queries themselves. Only IE 11 does not support this dynamic variable updating natively, but since it's a desktop browser we could easily transpile it to always use the desktop sizes. SectionI'm not sure I understand the correlation between |
|
Thanks @dbbk As discussed in the components working group on Monday, we're going to improve things incrementally and stage 1 is to sort out the issue with vertical margin. Next step is to look more closely at layout components or utilities that concern all types of spacing: margin and padding, both vertical and horizontal. As for CSS variables, this is again something worth looking into but out of the scope of this initial phase. With regard to your other feedback, I've removed the |
|
Ah fair enough, I wasn’t aware there was a component group meeting. Whoever
runs it please invite me in future.
…On Thu, 23 Aug 2018 at 9:40 am, Joe Bourne ***@***.***> wrote:
Thanks @dbbk <https://github.com/dbbk>
As discussed in the components working group on Monday, we're going to
improve things incrementally and stage 1 is to sort out the issue with
vertical margin.
Next step is to look more closely at layout components or utilities that
concern all types of spacing: margin and padding, both vertical and
horizontal.
As for CSS variables, this is again something worth looking into but out
of the scope of this initial phase.
With regard to your other feedback, I've removed the isFirst logic so
that should be clearer now, and I agree that the default value from the
original spacing utility makes no sense so I've removed that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AArcFBVlloGeqBgFjOm6wRXzDWZDKkWTks5uTmqFgaJpZM4WHIN7>
.
|
No description provided.