cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.07k stars 399 forks source link

[jss-default-unit] #525

Closed IsenrichO closed 7 years ago

IsenrichO commented 7 years ago

The JSS-Default-Unit package does not work properly for the line-height property when used in conjunction with the JSS-Expand plugin.

To clarify, the jss-expand docs show that the line-height property may be nested inside font as so:

font: {
  style: null,
  variant: null,
  weight: null,
  stretch: null,
  size: null,
  family: null,
  lineHeight: null
}

My config is given below:

import Aesthetic, { createStyler } from 'aesthetic';
import { create } from 'jss';
import Preset from 'jss-preset-default';
import JSSAdapter from 'aesthetic-adapter-jss';
import AdminTheme from './themes/admin';

const defaultUnitOptions = {
  'line-height': 'px',
};
const presetOptions = {
  defaultUnit: defaultUnitOptions,
};
const jssInstance = create(Preset(presetOptions));

const aestheticInstance = new Aesthetic(
  new JSSAdapter(jssInstance),
  { stylesPropName: 'classNames' },
);

aestheticInstance.registerTheme('admin', AdminTheme);

export { aestheticInstance as aesthetic };
export default createStyler(aestheticInstance);

I have found that when I specify a lineHeight that is not nested inside font, it works as expected. For example,

export const styler(theme => ({
  someThing: {
    fontSize: 16,
    lineHeight: 20,  // SUCCEEDS; this is mapped to `line-height: 20px;`
  },
})(MyComponent);

However, when nesting lineHeight within font (as jss-expand should allow), it fails to work properly. For example,

export const styler(theme => ({
  someThing: {
    font: {
      size: 16,
      lineHeight: 20,  // FAILS; is mapped to `line-height: 20;` (no pixel unit)
    },
  },
})(MyComponent);
kof commented 7 years ago

Because font-line-height prop is non-standard, it is handled separately https://github.com/cssinjs/jss-default-unit/blob/master/src/defaultUnits.js#L108

kof commented 7 years ago

Btw. lineHeight is a unit that can be unitless in CSS , consider not using defaults for this case.

IsenrichO commented 7 years ago

In general, I prefer to use line-height without units... or, at least that's what I learnt was a best practice for that particular CSS property. However, we are in the practice of always using it with pixels at my place of work. In that spirit, I was interested in setting a default unit for the property.

To clarify, are you saying that using font-line-height: 'px' should work?

kof commented 7 years ago

However, we are in the practice of always using it with pixels at my place of work.

Maybe you really shouldn't, line-height should stay in proportion to font-size and unitles value is the right way to do so.

To clarify, are you saying that using font-line-height: 'px' should work?

It should

IsenrichO commented 7 years ago

Awesome! Thanks for the help @kof !

IsenrichO commented 7 years ago

Having tried this now, I am finding that using 'font-line-height': 'px' does not work.

damianobarbati commented 6 years ago

Are grid-layout styles considered by default unit? I'm doing the following now: gridRowGap: theme.spacing + 'px'

kof commented 6 years ago

not yet https://github.com/cssinjs/jss-default-unit/blob/master/src/defaultUnits.js

damianobarbati commented 6 years ago

@IsenrichO how did you solve the lineHeight default as px issue?

Neither is working:

fontSize: 18,
fontLineHeight: 27, // not working, getting font-line-height: 21px;
fontSize: 18,
lineHeight: 27, // not working, getting line-height: 21; (the unitless instead of px)
HenriBeck commented 6 years ago

@damianobarbati you will need to explicitly set the default unit of the line-height prop to be pixels:

const defaultUnitOptions = {
  'line-height': 'px',
};
const presetOptions = {
  defaultUnit: defaultUnitOptions,
};
const jssInstance = create(Preset(presetOptions));
damianobarbati commented 6 years ago

Oh no... anything less verbose and not requiring to import-then-configure presets? 😞

All I'm doing right now is just:

import { jss, ThemeProvider } from 'react-jss';
kof commented 6 years ago

lineHeight is a unitless property in css by design, use it that way without unit, its better

damianobarbati commented 6 years ago

@kof you're totally right, using the default unitless: defintely more robust.