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

[v4.3] Including mixins multiple times shouldn't warn about RFS #28235

Closed
lucagouty opened this issue Feb 11, 2019 · 4 comments
Closed

[v4.3] Including mixins multiple times shouldn't warn about RFS #28235

lucagouty opened this issue Feb 11, 2019 · 4 comments
Labels

Comments

@lucagouty
Copy link

lucagouty commented Feb 11, 2019

Hi everyone, I'm opening this issue after a quick talk with @MartijnCuppens here : twbs/rfs#50 (comment)

Before 4.3.0, importing ~bootstrap/scss/mixins multiple times was ok, but now we get a warning :

WARNING: Watch out, RFS is included more than once!

Warning comes from scss/vendor/_rfs.scss imported in scss/_mixins.scss

In my opinion bootstrap should not warn for this, especially as there's no warning of that kind elsewhere (so doing it only for _mixins.scss seems weird, as at the end it won't generate any duplicated css).

Feel free to read my comment to rfs project for more details.

I'd be pleased to make a PR removing this warning, though as it's vendor/_rfs maybe we should do it in https://github.com/twbs/rfs instead.

Let me know, and have a good day guys 👋

@XhmikosR XhmikosR added the css label Feb 11, 2019
@lucagouty lucagouty changed the title Including mixins multiple times shouldn't warn about RFS [4.3] Including mixins multiple times shouldn't warn about RFS Feb 11, 2019
@lucagouty lucagouty changed the title [4.3] Including mixins multiple times shouldn't warn about RFS [v4.3] Including mixins multiple times shouldn't warn about RFS Feb 11, 2019
@mdo
Copy link
Member

mdo commented Feb 12, 2019

@MartijnCuppens This is an update for RFS and not Bootstrap itself, correct? Or should we aim to develop RFS here and publish changes back to the main repo? (Let's figure that out and document it on our side for future reference.)

@XhmikosR
Copy link
Member

I think it makes more sense to make any changes upstream first and then update it here.

@jcroll
Copy link

jcroll commented Feb 12, 2019

If including rfs multiple times is such a bad practice maybe it should be be decoupled from mixins.scss?

@MartijnCuppens
Copy link
Member

MartijnCuppens commented Feb 12, 2019

This functionality was originally added because RFS broke if it was included more than once. That's fixed in the meanwhile but I build in this warning to warn people if RFS was included multiple times in their config because they're probably doing something wrong. In the scope of RFS this is quite a handy feature, but a lot of people will not know what RFS is when they include Bootstrap.

Guess I'm just going to remove it, it was more a nice-to-have feature but if it confuses more than it helps, it's moot.

I'll make PRs in both repos.

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

No branches or pull requests

5 participants