-
Notifications
You must be signed in to change notification settings - Fork 30
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
base: main
Are you sure you want to change the base?
Conversation
classNames = classNames.concat(size.alias) | ||
size = size.value | ||
} | ||
return `.f${classNames.join(', .f')} { font-size: ${size}rem; }` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
- we have to validate the aliases globally so the user doesn't override his aliases across different scales
- 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'sclass="ferror"
you will always know (by tachyons naming conventions), this class has something todo with fonts
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
addressing my idea from #67