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

feat: rem based dimensions #80

Conversation

RobbyUitbeijerse
Copy link
Contributor

@RobbyUitbeijerse RobbyUitbeijerse commented Mar 31, 2023

hey there!

using rem as a unit compared to using pixels will make the web a much better place for visually impaired users, as typography and dimensions scale based on the scaling the user defined in their browser settings.

To get some info on the topic I can suggest these sources:

Our current project has a strong focus on accessibility - and while we created our own transformation on it I think the default provided by sd-transforms should be based on rem as well.

I excluded borders from this transformation as this can lead browser specific render issues and generally they offer no benefit to accessibility as borders aren't really content related, so there's no need for them to scale.

Let me know if you are open to the change so I can update the tests accordingly.

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

⚠️ No Changeset found

Latest commit: 5c536eb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@RobbyUitbeijerse RobbyUitbeijerse changed the title rem based dimensions feat: rem based dimensions Mar 31, 2023
@jorenbroekema
Copy link
Member

Hmm I don't think relative units are a good default for our transforms. Sometimes you need rem, sometimes em sometimes px (e.g. for a spacer that doesn't depend on base pixel size of the site). Pixels is an absolute unit in the context of web, and you can convert it to anything you want based on context. rem/em are relative to context (e.g. should it depend on body font-size, or container font-size?), context that this library cannot know about beforehand.

That said, I absolutely agree that for many semantic tokens, given you know some of the context (because they are semantic), transforming to em or rem might be good to do, but it's up to the user.

@RobbyUitbeijerse
Copy link
Contributor Author

That makes sense. What about shipping more options within the package though? So instead of changing the defaults, make people pick their own by simply mixing & matching transformations that are ready to go like..

    css: {
      transforms: [
        ...
        'ts/size/rem',
        ...
    ]

@jorenbroekema
Copy link
Member

jorenbroekema commented Apr 2, 2023

Yeah I think it makes sense, shipping a ts/size/rem transform in this package, as long as it's an opt-in choice by the user rather than a default. I should clarify btw, with "not by default" I mean not having it in the tokens-studio group by default, I'm okay with registering the transform as ts/size/rem when users run the registerTransforms function of this library, just not include it in the tokens-studio transformGroup.

How do we easily pass the baseFontSize option btw? As far as I know, you can't easily pass options to transforms except for:

import { transformRem } from '@tokens-studio/sd-transforms';

StyleDictionary.registerTransform({
  name: 'ts/size/rem',
  type: 'value',
  matcher: token =>
    ['sizing', 'spacing', 'borderRadius', 'fontSizes', 'dimension'].includes(
      token.type,
    ),
  transformer: token => transformRem(token.value, 24), // <-- pass 24 as base font size 
});

Maybe that's acceptable, I'm fine with it personally.. for now. I think we need a better way in the future though. More on that at the bottom of this comment, but not required to address in this PR.

If this sounds good to you, let's adjust the PR to not register it by default and change the function signature to:

export function transformRem(tokenValue: string | undefined | number, baseFontSize = 16): string | undefined {}

to simplify things a bit, as baseFontSize is the only option, the only foreseeable one for this transform I would say.

What we would still need to finish the PR then is to:

  • Write tests for transformRem function
  • Add to the README.md, with a note saying it's not added to the tokens-studio transformGroup, it's opt-in, and that in order to pass a different baseFontSize than 16, you'll have to register it under a different transform name yourself and change this value in the transformer prop (see my code snippet above)
  • Add a changeset entry (a human readable changelog, npx changeset, follow the CLI prompts), but I can do it as well if needed.

Lastly, would you be down also contributing a "ts/size/em" transform, they could use the same code mostly, just a slightly different unit and registered transform name. I just realized em doesn't make sense in a transform. StyleDictionary runs on a global context with regards to your design tokens, and em means it is relative to a local container. rem as a transform makes sense because it's global to the body's font-size.

Not for this PR, but in the future

See issue: #87
If we were to implement the above proposed feature, we could probably do with only a single transform ts/size/dimension and pass the options of "what unit" and "what base font size" in the token/token group metadata, which I think in the end is the cleanest way to do it, and it can be controlled also from the Tokens Studio Figma Plugin by designers. This would make ts/size/px/ts/size/rem/ts/size/em obsolete, meaning a breaking change, so it's preferable to have it done before we hit v1.0.0

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.

2 participants