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

Refactor to make adding more CSS props simple; add support for variable character width & letter-spacing #35

Closed

Conversation

tinymachine
Copy link
Collaborator

@tinymachine tinymachine commented Mar 4, 2019

Had a bit too much spare time, so I may have gone overboard with this PR. Totally understand if you need to reject it if it's too broad.

Major change: apply defaults, calculate and adjust property values programmatically

The biggest update here is in 7d26406, which refactors how CSS properties are handled by the script. Instead of 'manually' handling all calculations (with separate lines of code for applying defaults for each property, and calculating and applying each property value), all supported properties (and any defaults) are defined in a config var. Then defaults are applied automatically for all included properties, and all calculations are handled programmatically. This way when you want to support additional CSS properties, you just update the config, without having to worry about making additional per-property updates throughout the script.

For example, adding letter-spacing support to the script is now as simple as adding the following to config.supportedProps:

{
  propName: 'letterSpacing',
  unitsDefault: 'em'
}

Adding per-property defaults for minWidth and maxWidth is supported as well, of course. Here's the entire config for all five props supported in the PR:

supportedProps: [
  {
    propName: 'fontSize',
    minWidthDefault: 1.0,
    maxWidthDefault: 1.8,
    unitsDefault: 'em'
  },
  {
    propName: 'fontStretch',
    unitsDefault: '%'
  },
  {
    propName: 'fontWeight'
  },
  {
    propName: 'letterSpacing',
    unitsDefault: 'em'
  },
  {
    propName: 'lineHeight',
    minWidthDefault: 1.33,
    maxWidthDefault: 1.25
  }
]

Limitations

  1. Each supported property must have a propName that matches the JS equivalents for CSS properties. The script assumes the propName is what's used by the API params, so the letter-spacing params would (as currently coded) have to be min/maxWidthLetterSpacing (the script is aware of the initial cap on the property name) and letterSpacingUnits. Params for changing character width would (as currently coded) have to be min/maxWidthFontStretch and fontStretchUnits. This could be changed pretty easily to handle param names that don't necessarily map directly to their CSS property, but it might be a good thing to enforce a standard style on the params and to make explicit exactly what the params control.

  2. Special properties like font-variation-settings, which can control multiple properties at once, aren't supported. This is probably fine, though, since the VF variation axes users will probably generally want to control (and the ones more variable fonts support) would be more standardized variation axes like weight and sometimes width, which honor their own respective CSS properties (e.g. font-weight and font-stretch).

Other updates

`font-weight` works just like `font-variation-settings: "wght"`, but unlike the latter it doesn't override the CSS cascade (which happens in Chrome and Safari when the `@font-face` declaration includes a `font-weight` range). (See note on https://developer.mozilla.org/en-US/docs/Web/CSS/font-variation-settings: 'font characteristics set using `font-variation-settings` will always override those set using the corresponding basic font properties, e.g. `font-weight`, no matter where they appear in the cascade.') So, if some text within a Textblock block has a specified weight (e.g. for `<strong>` tags), that won't be overridden by the script like it would previously have been.
…ight`

More accurately conveys the actual property / variation axis being manipulated.
(Leave reference to 'simulating grades' in readme untouched, though.)
Move it from bottom of Parameters section to top intro.
Make explicit the relationship between container width and the type-related parameters.
- Use a top-level `config` var to contain all defaults and list all supported type-related properties.

- Allow for units to specified per-property (e.g. `fontSizeUnits`, though `units` is still respected and applied only to `fontSize`).

- Apply defaults and calculate all CSS property values programmatically instead of handling each property manually. This makes supporting new CSS properties trivial.
Include new font (AmstelvarAlpha), which supports variable width. Also add legacy `units` param to the legacy test.
Newer versions allow for var names and even property names to be 'mangled' when minified (i.e. shortened from longer human-friendly names to single characters).
Enable mangling object property names (i.e. shortening longer human-friendly names to single characters). (This commit also includes some Prettier automatic code-formatting.)
No need for the JS file to be minified every time the demo files update.
(Prevents meaningless version control changes from being indicated when script is re-minified but no code has changed other than the date.)
Replace with built-in JS `forEach()` and `document.querySelectorAll()` methods.
@tinymachine tinymachine changed the title Simplify config of props [DRAFT] Simplify config of props Mar 4, 2019
@tinymachine tinymachine changed the title [DRAFT] Simplify config of props Refactor to make adding more CSS props simple; add support for variable character width & letter-spacing Mar 4, 2019
@tinymachine tinymachine marked this pull request as ready for review March 4, 2019 08:20
- Simplify `run()` function
- Change `calc()` into `calcCurrentPropValue()` to calculate a single property at time
- Separate width-ratio calculation into its own `calcCurrentWidthRatio()` method.
This eliminates hard-coding 'Units' throughout the script. `fontSizeUnits` is no longer hard-coded in script, so remove it from the list of reserved properties in gruntfile.js.
@tinymachine tinymachine force-pushed the simplify-config-of-props branch from 8ba9b57 to 17ffde9 Compare March 4, 2019 20:16
@tinymachine
Copy link
Collaborator Author

Sorry, squashing some commits to make this PR easier to review. Will re-submit it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants