fkhadra / react-toastify

React notification made easy 🚀 !
https://fkhadra.github.io/react-toastify/introduction
MIT License
12.58k stars 692 forks source link

[POLL]: Remove glamor and switch back to css? #146

Closed fkhadra closed 6 years ago

fkhadra commented 6 years ago

Simply click on your choice. Thanks.

Ky6uk commented 6 years ago

I think an imposing the only one CSS-in-JS solution is a bad idea in 2018. There are a lot of existing different solutions including the old-way CSS files which every developer should be free to choose byself.

fkhadra commented 6 years ago

Hey @Ky6uk,

Could you share an example of others solutions? I'm open to everything.

Edit: sometimes I think that going back to scss is the best things to do.

Ky6uk commented 6 years ago

@fkhadra I meant CSS-in-JS solutions. There are a lot of them. styled-components, JSS, aphrodite, etc. And every developer should be able to use any of them (or just classnames as is) without an imposing by third-party libraries.

For example I got collisions when I tried to use my own classnames with react-toastify. In the dev environment there are no problems when I tried to use classname for overriding default toast classnames. But in the production environment I got problems with it. That was the problem with styles precedence with speedy mode inside glamor library. So I was forced to use glamor in this case when it's not using on the codebase at all.

fkhadra commented 6 years ago

@Ky6uk you're totally right about that. I have to figure out if there is a way to do that. Do you have any idea how it can be done?

Ky6uk commented 6 years ago

I am not well-good with css-in-js libraries itself. I think including a some kind of css files is still the best solution. Developer will be able to make a decision to include that built-in style in his project or use his any other solution (including any of css-in-js libraries).

fkhadra commented 6 years ago

Using css like it's done with react-virtualized seems to be the best choice. It also removes a dependency.

The only drawback I guess is that end users gonna need to import the style file. Anyway, I don't think it's a big deal.

Ky6uk commented 6 years ago

Yeah, I think so. You can always bump the major version's number because this will the breaking change.

ghost commented 6 years ago

I agree, move back to normal stylesheets or scss. I get that glamour's the cool kid but how are we supposed to override or extend from react-toastify when the classes are meaningless?

Here's an example of what Webpack does to a react-toastify after its stylesheet is imported:

screen shot 2018-04-02 at 4 03 50 pm

So, to override classes I just end up putting a custom class name on all toasts -- so we're right back to regular old CSS anyway.

toastClassName="my-toast"

But that's not the best solution either. In the past, if I wanted all of the toasts that are position="bottom-center" to look different, I just needed to override one style:

.toastify--bottom-center {
    bottom: 3em;
  }

Now, because of glamor, I need to comb through the codebase for all toast calls that use bottom-center and add yet another conditional class. So, the style rules and class names end up being dispersed everywhere instead in one predictable location -- a stylesheet.

I would at least give people the option of using a glamour or scss implementation of react-toastify.

fkhadra commented 6 years ago

Hey @nbcnc,

Thanks for your feedback. I expect to launch the v4 this week if everything goes well. The v4 switch back to css.

ghost commented 6 years ago

great to hear @fkhadra thank you!

petewilkins commented 6 years ago

Hey @fkhadra, thanks for working on react-toastify. Is this still planned to be released this week?

fkhadra commented 6 years ago

Hey @petewilkins,

Yes, I'm almost done. I need to update the documentation and add more tests.

Thanks for your support.

goenning commented 6 years ago

I just found this package and got surprise by its size (60kB minified). It seems like glamour is responsible for 50% of it:

https://bundlephobia.com/result?p=glamor@2.20.40 -> 29.6 kB
https://bundlephobia.com/result?p=react-toastify@3.4.3 -> 61.3 kB

I totally agree with this change and curious to see numbers of v4! Great work @fkhadra 👍

fkhadra commented 6 years ago

@goenning

screen shot 2018-04-14 at 21 54 42

If I refer to Webpack output 30kB minified and around 8kB gzipped. I think that I could reduce the size if I remove my custom prop-types.

fkhadra commented 6 years ago

You can already test the v4:

yarn install react-toastify@latest

Feedback is welcome

goenning commented 6 years ago

@fkhadra I believe you forgot to push the css to the NPM package, I just got this:

Module not found: Error: Can't resolve 'react-toastify/dist/ReactToastify.css'

ls node_modules/react-toastify/dist/
ReactToastify.js      ReactToastify.js.map
fkhadra commented 6 years ago

@goenning holy ... you're right. I'm on it.

fkhadra commented 6 years ago

@goenning Fixed. Thanks for that.

goenning commented 6 years ago

@fkhadra It's working now! Thanks for quick reply. I got a total reduction of 23kB on this version, because CSS increased by 7kB.

I'll continue using rc version and let you know if I find something else 👍