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

Remove explicit sass usage and add bootstrap as a dependency #15

Closed
wants to merge 5 commits into from
Closed

Remove explicit sass usage and add bootstrap as a dependency #15

wants to merge 5 commits into from

Conversation

vbalazs
Copy link

@vbalazs vbalazs commented Apr 6, 2019

Builds on #14 (hence the commits in the diff)
Fixes #13

I'm still unsure about adding bootstrap as a dependency because this way we might lock the version and users cannot upgrade it independently if 4.4.x comes out. :| But this seems more correct to me.
As a middle ground, we could be more liberal and set the version as ~> 4.3? 🤔
What do you think?

@lightyrs
Copy link
Owner

@vbalazs Have you tried running the update rake task with your new code. Does it work correctly?

@vbalazs
Copy link
Author

vbalazs commented Apr 10, 2019

@lightyrs yep, as far as I can judge the output. It didn't fail and updated many files, also the value of TABLER_SHA in lib/tabler/rubygem/version.rb

@rbngzlv
Copy link

rbngzlv commented May 13, 2019

Hi @vbalazs and @lightyrs,

I'm not sure to adding bootstrap as a runtime dependency in the gem, because the user can provide it in another way, for example, we are loading bootstrap from node_modules and not the bootstrap gem, so we will end with two bootstrap versions installed.

// app/assets/stylesheets/application.scss
@import "tabler/variables"; // this gem
@import "bootstrap/scss/bootstrap"; // from node_modules
@import "tabler"; // this gem

I forked the @vbalazs fork and made a branch based on their other PR #14, to fix the sass usage, this is de diff: https://github.com/vbalazs/tabler-rubygem/compare/fix-dev-env...rbngzlv:rbngzlv/remove-sass

What do you think?

P.D: thank you both for the work on this

@lightyrs
Copy link
Owner

This seems like a legitimate issue @rbngzlv — Please submit your branch as a PR.

@vbalazs vbalazs closed this Sep 18, 2019
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.

Error when using sassc-rails
3 participants