[tools.rem] Remove rem-function? #204

Closed
csshugs opened this Issue Oct 21, 2016 · 14 comments

Projects

None yet

6 participants

@csshugs
Member
csshugs commented Oct 21, 2016 edited

As mentioned several times before, I'm in favor of removing the inuit-rem function and mixin, i.e. the whole partial.

I'm not saying, that this is useless in general. Quite the contrary. But we don't consider inuitcss as a mixin library and will not provide a whole lot of functionality which we don't need in the first place for the framework itself. And since this is just used in the _example.components.buttons.scss and in _generic.shared.scss, IMO we can dump it:

// _example.components.buttons.scss

.c-btn--small {
  padding: $inuit-global-spacing-unit-tiny $inuit-global-spacing-unit-small;
}

.c-btn--large {
  padding: $inuit-global-spacing-unit $inuit-global-spacing-unit-large;
}
// _generic.shared.scss


address,
h1, h2, h3, h4, h5, h6,
blockquote, p, pre,
dl, ol, ul,
figure,
hr,
table,
fieldset {
  margin-bottom: $inuit-global-spacing-unit;
}


/**
 * Consistent indentation for lists.
 */

dd, ol, ul {
  margin-left: $inuit-global-spacing-unit;
}

The way it is used in the buttons component, it's not IE8 compatible, because it just outputs a rem value without px fallback. I know, it's just an example file, but as a matter of fact, in that specific case, you could not use the inuit-rem mixin like this:

.c-btn--small {
  @include inuit-rem(padding, $inuit-global-spacing-unit-tiny $inuit-global-spacing-unit-small);
}

because the mixin accepts no shorthand declarations, i.e. in this case we would have to write:

.c-btn--small {
  @include inuit-rem(padding-top, $inuit-global-spacing-unit-tiny);
  @include inuit-rem(padding-bottom, $inuit-global-spacing-unit-tiny);
  @include inuit-rem(padding-left, $inuit-global-spacing-unit-small);
  @include inuit-rem(padding-right, $inuit-global-spacing-unit-small);
}

which makes the mixin kind of obsolete in those cases and you have to go back to the inuit-rem function in those circumstances.

Apart from that, I wonder why we use rem units at all in generic.shared? Because in all other modules where margins are assigned, we are just declaring px values. I don't see why in this instance there need to be rem values.

@jamielovelace

+1 - you could always use a pix to rem gulp plugin to sort the fallbacks out on compile anyway

@florianbouvot
Member
florianbouvot commented Oct 24, 2016 edited

@csshugs I agree to remove tools.rem.

But I'm wondering if it's best to only use px value or use both px + rem (or just rem value) in inuitcss (and in my components)

😔

@csshugs
Member
csshugs commented Oct 24, 2016

As mentioned somewhere else before, rem-units make most sense when font-sizes are concerned, which we cover with our inuit-font-size mixin. I (IMO) don't see any advantage in using rem units for margin-bottom etc. so I'm in favor of using just px values for that. If someone wants to combine margin declarations with rem values, there is postCSS to help them achieve that.

@csshugs csshugs added this to the 6.0.0-beta.5 milestone Oct 26, 2016
@herzinger
Contributor

So... Was this decided? Shouldn't tkt-0204 get a pull request, be merged and this issue closed?

@florianbouvot
Member
florianbouvot commented Nov 11, 2016 edited

I create this little jsfiddle demo to explain my point of view: with px unit, if a user changes the default font size of his browser, vertical rhythm will be broken.

(Open JSFiddle then change the font size of your browser)

@inuitcss/core thoughts?

@herzinger
Contributor
herzinger commented Nov 11, 2016 edited

It certainly breaks proportion, but that's because of inconsistencies. if this is a concern (and I'd not see any problem if it is), we should adopt rem in all margins and paddings throughout the project, including spacing utilities and boxed objects.

@csshugs
Member
csshugs commented Nov 11, 2016 edited

@florianbouvot Agree.

@csswizardry did mention anywhere else (I can't find it right now) that in this case, the inuitcss users can use tools like postCSS to turn the px values into rem values. If that's our opinion to handle this topic for inuitcss in general, I think think we should drop the rem function and mixin.

If not and we want to provide rem units already out of the box in the core framework, we shouldn't not only decline this issue and leave everything as is, but on top of that take care of all the other instances (some objects and spacings) where margins are declared and provide rem units there as well.

I'm for the first approach due to simplicity.

@florianbouvot
Member
florianbouvot commented Nov 11, 2016 edited

We can start with px and take time later (after beta) to test rem... 😉

@herzinger
Contributor

I think it's more of a matter of avoiding unnecessary complexity than of volume of work.

@herzinger
Contributor

The actual question should be: do we want to advocate the use of rem, or it's just an optional nicety? If it's the later, we shouldn't bother. PostCSS does the trick.

@nenadjelovac
Member

I'm for the first approach due to simplicity.

I agree with @csshugs, even though it's not ideal solution (as @florianbouvot pointed out in his demo).

@anselmh
Member
anselmh commented Nov 13, 2016

Wait, wait. Unfortunately, there seems to be a misunderstanding about PostCSS and rem conversion here. There is a difference in using a rem value initially and convert it to a px value and doing this vice-versa. If you increase the base font-size, a px size will be still the same while a rem size would be calculated again based on the new base size.

Given how we used the rem mixin in the buttons example file, this doesn’t apply. Yet, this doesn’t mean that we can just swap it out and advertise that you can achieve the same by using a PostCSS plugin.

Also, please reconsider the timeline regarding this. Given the impact and differences between px and rem/em, we cannot change this behavior anymore in inuitcss v6.0.0 since it would be a breaking change, requiring a new major version after leaving beta flag according to the semver guidelines which we stick to.

By the way I implicitly mention em here as well since it would be most useful to build inuitcss based on rem (for fixed sizing) and em (relative sizing, e.g. button padding) to achieve a simpler, self-scaling codebase.
Indeed, we could even introduce em right now for relative sizing as it can of course be used with px as a base and is compatible to all browsers.

I don’t want to be the one holding back here but I think these are valid concerns that we should rethink first.

@csshugs
Member
csshugs commented Nov 14, 2016 edited

@anselmh

We can start with px and take time later (after beta) to test rem...

As @florianbouvot correctly stated here, IMO we can for now merge #232 and take care about rem later (I for myself need at least a couple more weeks to think about this whole rem-thing). I think it's fine to roll out the major version without providing rem units for everything beyond font-sizing.

There is a difference in using a rem value initially and convert it to a px value and doing this vice-versa.

It's not about converting initially set rem units into px values (via a possible PostCSS plugin), but rather the other way round. I'm not sure if I missed something, but this way, it's a secure way to just leave rem out for now (with #232) and (maybe) later add a cool and robust solution for rem units everywhere, right?

@csshugs
Member
csshugs commented Nov 15, 2016

#232 is merged

@csshugs csshugs closed this Nov 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment