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

Refacto update #37

Merged
merged 5 commits into from Aug 11, 2018
Merged

Conversation

GuillaumeCisco
Copy link
Contributor

Big refactoring - Fix #36 #35 #29 #23 #2
Add className property for overriding loader style.
Fix scalibility pacman loader.
Load spinner like lodash do : import {BarLoader} from 'react-spinners or import BarLoader from 'react-spinners/BarLoader'

@davidhu2000
Copy link
Owner

Thanks @GuillaumeCisco, quick question, with the new updated import, does it allow both

import { Barloader } from 'react-spinners';
import BarLoader from 'react-spinners/BarLoader';

@GuillaumeCisco
Copy link
Contributor Author

GuillaumeCisco commented Jul 14, 2018 via email

@davidhu2000
Copy link
Owner

davidhu2000 commented Jul 23, 2018

@GuillaumeCisco looks like merging #34 caused a ton of merge conflicts.

#34 added a prop for loaderStyle, which I think is similar to what you are trying to do with className. I'm not sure which is better

@GuillaumeCisco
Copy link
Contributor Author

@davidhu2000 Yes I saw something like this.
Using a new defined loaderStyle is over-killing. As we use emotion, the simplest and proper way is to stick with emotion api. Using className is easy, scalable, maintainable because it is a fundamental part of emotion.

Simply passing your css style overriding everything is far more easier and understandable without learning a new prop.

README should be updated too, in order to show a example.

@ludofischer
Copy link

Hi, I’m also interested in importing Spinners one at a time. Could we leave loaderStyle vs className aside for the moment and try to merge the file structure refactoring? I could prepare a new pull request based on this one but without touching loaderStyle.

@GuillaumeCisco
Copy link
Contributor Author

@ludovicofischer @davidhu2000
I've just updated the code for solving conflicts and removing loaderStyle

@ludofischer
Copy link

Great! But technically the next release becomes breaking now. I don’t use loaderStyle so I don’t care and according to semver, when the major version is 0 anyone should expect breakage at any moment, but it could annoy someone out there. I guess @davidhu2000 can decide.

@GuillaumeCisco
Copy link
Contributor Author

@ludovicofischer , yes absolutely.

@davidhu2000
Copy link
Owner

This is definitely at least a minor version bump, may have to deprecate 0.3.3

Copy link
Owner

@davidhu2000 davidhu2000 left a comment

Choose a reason for hiding this comment

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

👍

@davidhu2000 davidhu2000 merged commit 0fa985c into davidhu2000:master Aug 11, 2018
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.

None yet

3 participants