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

Make it up with sass-resources-loader #357

Merged
merged 6 commits into from
Oct 31, 2018

Conversation

x-yuri
Copy link
Contributor

@x-yuri x-yuri commented Sep 6, 2018

Fixes an issue discovered by author in #285 after upgrading boostrap-loader.

This change is Reviewable

@justin808
Copy link
Member

Can you add an example to https://github.com/shakacode/bootstrap-loader/tree/master/examples?

I'll merge this once I have a small sample where I can verify this.

Also, please confirm that the all the examples, https://github.com/shakacode/bootstrap-loader/tree/master/examples, still work correctly.

Bonus points for adding an entry to the CHANGELOG.md.

@x-yuri
Copy link
Contributor Author

x-yuri commented Sep 7, 2018

Can you add an example to https://github.com/shakacode/bootstrap-loader/tree/master/examples?

@justin808 By example you mean another directory in examples dir with an app, that makes use of sass-resources-loader? That can be built with Bootstrap 3, 4 for development or production environment?

Also, please confirm that the all the examples, https://github.com/shakacode/bootstrap-loader/tree/master/examples, still work correctly.

By still work correctly you mean that page appearance doesn't change?

I'll merge this once I have a small sample where I can verify this.

If you just want to confirm, that sass-resources-loader doesn't work without a PR, I can provide here an archive with an app that demonstrates just that.

@justin808
Copy link
Member

Can you add an example to https://github.com/shakacode/bootstrap-loader/tree/master/examples?

@justin808 By example you mean another directory in examples dir with an app, that makes use of sass-resources-loader? That can be built with Bootstrap 3, 4 for development or production environment?

Sounds good. Pick any example and add the sass-resources-loader.

Also, please confirm that the all the examples, https://github.com/shakacode/bootstrap-loader/tree/master/examples, still work correctly.

By still work correctly you mean that page appearance doesn't change?

I'll merge this once I have a small sample where I can verify this.

If you just want to confirm, that sass-resources-loader doesn't work without a PR, I can provide here an archive with an app that demonstrates just that.

Doesn't crash during build or running. No console errors. Looks about the same.

@x-yuri
Copy link
Contributor Author

x-yuri commented Sep 11, 2018

@justin808 I'm not sure I understand what's the idea of adding sass-resources-loader example. There are 3 types of examples I could think of:

  1. Example that confirms that bootstrap-loader works with sass-resources-loader. I'd made it an archive with an app attached to this page.

  2. Example that shows users how to use bootstrap-loader. These kind of examples go to the examples dir.

  3. Example that shows users how to use sass-resources-loader. examples dir (and this repository) doesn't seem like a good place for these. And that's what you seem to want me to do.

So let me show you the diff first. Do you want another example in examples dir that differs this little?

@justin808
Copy link
Member

@x-yuri see #362

I'm a bit confused on why you want to have the sass-resources-loader in your bootstrap config for the non-css-modules example? Maybe the bugfix is fine, but the example for basic doesn't make sense?

@x-yuri
Copy link
Contributor Author

x-yuri commented Oct 15, 2018

I'm a bit confused on why you want to have the sass-resources-loader in your bootstrap config for the non-css-modules example?

@justin808 Let's start with the use case. I should have mentioned it from the beginning. I want to add Bootstrap to my site. I'm already using sass-resoruces-loader. So, I've got a resource file with handy variables and mixins. After I've added Bootstrap with the help of bootstrap-loader, I realize I need to customize it a bit (preBootstrapCustomizations, bootstrapCustomizations). And I want to use there (bootstrap/customizations.scss) all those handy variables and mixins. Without this PR I can't add sass-resources-loader to styleLoaders array.

So if you want example specifically about sass-resources-loader, we've got to add customization file and use there some variables or mixins from resources file. But there's nothing special about sass-resources-loader, except for bootstrap-loader choking on it the way it is now. Example that uses any other loader would do. All the examples use one loader or another. So we've got that covered already, aren't we? ;)

Why not css-modules example? Because it has nothing to do with CSS modules. It has to do with using styleLoaders parameter, not even sass-resources-loader.

The problem with your example the way I see it is that it shows how to use sass-resources-loader with React, not with bootstrap-loader.

Hopefully that clarifies my take on it. But if you still want an example, I can try to modify css-modules example to make it use customization.scss, that uses variables from resource file with the help of sass-resources-loader. Or you can go ahead with your PR. Whatever works best for you.

@justin808
Copy link
Member

@x-yuri You seem to understand the issue way better than me.

Hopefully that clarifies my take on it. But if you still want an example, I can try to modify css-modules example to make it use customization.scss, that uses variables from resource file with the help of sass-resources-loader. Or you can go ahead with your PR. Whatever works best for you.

Please go ahead and modify the example. I'll try it out and merge it, and cancel my PR.

@x-yuri x-yuri force-pushed the sass-resources-loader branch from 2b1ec52 to 4bea26d Compare October 27, 2018 13:31
@x-yuri
Copy link
Contributor Author

x-yuri commented Oct 27, 2018

@justin808 Okay. Hopefully that's all there is to it.

multiple-entries example is broken as you might know. The README.md is outdated, and even if I follow the steps, npm run install-local fails.

yarn install has changed the lock files, particularly (basic example):

 "bootstrap-loader@file:../..":
-  version "2.2.0"
+  version "3.0.1"
   dependencies:
     chalk "2.4.1"
     escape-regexp "0.0.1"

Not sure if that's an issue.

@justin808
Copy link
Member

@x-yuri Hey, I really appreciate your patience here. I loaded the code and tried to see a change when I changed $custom-border-radius: 50px; to some other value in both the bs3:dev and bs4:dev setups.

Can you please provide some instructions in https://github.com/shakacode/bootstrap-loader/blob/master/examples/basic/README.md as how sass-resources-loader works. I wanted to see a change in the UI.

You can add an image to this PR in a comment and copy the markdown from the comment into the README.md file.

Also, should this be 3.0.2 or 3.1.0? Can you please give me your desired changelog entry: https://github.com/shakacode/bootstrap-loader/blob/master/CHANGELOG.md.

image

@x-yuri
Copy link
Contributor Author

x-yuri commented Oct 31, 2018

@justin808 Bootstrap developers have a bit different idea of what alert is. You can see it at the bottom of the basic example's page:

2

Anyways, I've added the description to the README as you asked.

Regarding the version, I think it's a bug, so 3.0.2 makes more sense to me. As for CHANGELOG entry, I'd say:

Fixed

  • Make it work with sass-resources-loader

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong: Thanks!

Reviewed 14 of 14 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@justin808 justin808 merged commit eb8159a into shakacode:master Oct 31, 2018
@justin808
Copy link
Member

Released in 3.0.2.

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.

2 participants