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

Declare font-size mixin for Less and add tests for font-size mixin #129

Merged
merged 3 commits into from
Apr 2, 2019
Merged

Conversation

tsilvap
Copy link
Contributor

@tsilvap tsilvap commented Mar 28, 2019

The font-size mixin doesn't work for Less and Stylus right now because it was never declared.

@tsilvap tsilvap requested a review from MartijnCuppens as a code owner March 28, 2019 23:36
@MartijnCuppens
Copy link
Member

Good point! It seems like the font-size() mixin isn't used in our tests either.

The tests for this PR failed for stylus (See https://travis-ci.org/twbs/rfs/jobs/512764679#L509). It seems like this line in the Stylus docs is causing this:

If we wished, we could invoke stripe() as if it were a property:
stripe #fff #000

Which means the font-size property always is overridden by the font-size mixin. We could probably fix this by using unquote('font-size') but will still cause the mixin to trigger if the font-size property is used in any custom css.

So we got 2 options for Stylus:

  • Add the Stylus mixin anyway and add a warning to the readme. This could break existing implementations though. I'm not sure to what extent since this is the first time this is reported.
  • Don't add the mixin and change it's implementation in the readme.

For less everything looks ok. We still need to add some tests, but the implementation depends on the direction we take with Stylus.

@tsilvap
Copy link
Contributor Author

tsilvap commented Mar 30, 2019

Which means the font-size property always is overridden by the font-size mixin.

That is a bummer. I don't think overwriting the font-size property is a good idea, one of the main reasons I was eager to adopt rfs is because you can adopt it "incrementally", that is, without breaking fonts that you've previously made responsive manually, for example. But there may be other things to consider.

In the meantime, I've removed the font-size mixin from the Stylus file and I've written the tests for the font-size mixin. I don't know if that is the best way to write them, what do you think?

@tsilvap tsilvap changed the title Declare font-size mixin for Less and Stylus Declare font-size mixin for Less and add tests for font-size mixin Mar 30, 2019
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

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

I don't know if that is the best way to write them, what do you think?

This is the best way I guess. Thanks for also taking care of the tests, @tsilvap!

@MartijnCuppens MartijnCuppens merged commit 62fef17 into twbs:master Apr 2, 2019
MartijnCuppens added a commit that referenced this pull request Apr 2, 2019
MartijnCuppens added a commit that referenced this pull request Apr 3, 2019
* Remove font-size mixin in docs for stylus

See #129 (comment) for additional info
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.

3 participants