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

Include Sass files in npm package and allow $spinnerSize Sass variable to be reassigned #81

Merged
merged 4 commits into from
May 15, 2018

Conversation

zalog
Copy link
Contributor

@zalog zalog commented Jan 25, 2018

There are 2 commits:

  • solve this in a way, by isolating transition to padding, and not transitioning background-color and color.

  • adding !default to sass variable which allows reassigning.

Hope it helps.

@zalog zalog changed the title fix: bootstrap 4 compatibility fix: bootstrap compatibility and var assign Jan 25, 2018
@theodorejb theodorejb force-pushed the 2.x branch 3 times, most recently from 99f0f2f to fcb89b5 Compare May 13, 2018 18:57
@theodorejb theodorejb closed this May 13, 2018
@zalog
Copy link
Contributor Author

zalog commented May 14, 2018

What happened here? Do you need some help?

@theodorejb theodorejb changed the base branch from 2.x to master May 14, 2018 15:01
@theodorejb
Copy link
Collaborator

theodorejb commented May 14, 2018

It looks like the base branch was set to 2.x, so when I removed this branch GitHub closed your pull request.

@theodorejb theodorejb reopened this May 14, 2018
@zalog
Copy link
Contributor Author

zalog commented May 14, 2018

Oh, now I see. This branch got some mess :)
I'll come back soon with a clean rebase

@theodorejb
Copy link
Collaborator

I fixed it for you. :)

I'm trying to understand what issue this PR fixes, though. You can test Ladda with Bootstrap 4 here, and it seems to work fine: https://theodorejb.github.io/Ladda/test/bootstrap4.html.

@zalog
Copy link
Contributor Author

zalog commented May 14, 2018

Sorry, I can't replicate that collision anymore...
It's been a long time since january, I'm getting old :(

We can keep the other commit.

@theodorejb
Copy link
Collaborator

For overriding the default spinner size, wouldn't it be better to use the built-in data-size attribute, or else create a custom CSS class for your buttons? The Sass files are excluded from the Ladda 2.0 npm package, anyway.

@zalog
Copy link
Contributor Author

zalog commented May 14, 2018

Oh, I see.
But imho I don't think excluding sass files are a good ideea :(
I think there are a lot of devs that use sass files and variables.

I recently made this (scroll down to #via-npm) and I think letting people customise that from pre-processors is better. Like bootstrap :)

Of course, we can close this pr if there will be no sass config.

@theodorejb theodorejb changed the title fix: bootstrap compatibility and var assign Include Sass files in npm package and allow $spinnerSize Sass variable to be reassigned May 15, 2018
@theodorejb
Copy link
Collaborator

I reverted the unnecessary Bootstrap 4 commit, and added a commit to re-include the Sass files in the npm package. Thanks for the contribution!

@theodorejb theodorejb merged commit 11d7ef8 into hakimel:master May 15, 2018
@zalog
Copy link
Contributor Author

zalog commented May 15, 2018

Great! Thanks for reconsider :)

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