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

add alias option to typescale #68

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ctaepper
Copy link

@ctaepper ctaepper commented Nov 3, 2017

addressing my idea from #67

classNames = classNames.concat(size.alias)
size = size.value
}
return `.f${classNames.join(', .f')} { font-size: ${size}rem; }`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% absolutely love this. Only thing I wonder about is if we make the alias not auto prefixed with f?

So:

{value: 1.5, alias: ["copy", "error"]}

Becomes

.f3, .title, .copy

And if you desire the f prefix you can add in the aliases: ["fcopy", "ferror"]. Might make it a touch more flexible. Thoughts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR for typescale is just a proof of concept. if we implement this, it should be generally available for all scales (spacings, heights, widths, etc...).
so omitting the class prefix leads to 2 problems:

  1. we have to validate the aliases globally so the user doesn't override his aliases across different scales
  2. it will be harder to understand the classes when reading them in html code, because if it says class="error", you have no idea what exactly this class is defining. if it's class="ferror" you will always know (by tachyons naming conventions), this class has something todo with fonts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, definitely agree it should be handled for all the things. I think we can look for identical aliases and warn when that occurs while also piping the output css through immutable-css to log any conflicting class names.

I'd like to see the ability to actually "eject" from the Tachyons naming conventions if they so choose, but that's prolly a bit different than aliasing. So perhaps aliases always are prefixed with the class base and one can actually change the class base when they desire something different:

{
  typeScale: {
    values: [1,2,3, { value: 4, alias: ['big', 'headline' }],
    classBase: 'heading'
  }
}

I'll do some thinking on this over the weekend, as it'd be great to come up with a config that can handle all of this while still maintaining a sensible config api.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I was wondering, why do you handle spacing different then the other settings?
https://github.com/tachyons-css/tachyons-generator/blob/master/lib/spacing.js#L54

with this step/ratio setting, we wouldn't be able to apply this proposal

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that was a poor api choice on my behalf. I think we should prolly move it to become inline with everything else (which should be okay since we're not 1.0.0 yet).

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.

None yet

2 participants