Remove Emotion and add inline css #127

stefanoruth · 2020-05-12T14:24:28Z

This i my proposal at the moment to remove emotion, i would very much like to hear what you think about it. :)

It inlines the needed css inside the bundle, and i have added an option to disable the inline css, so people can add it to their own css file, and bundle it the way they want it.

I havent added any test to the new helper function let me know if you want that too,

I would also like to take a look at the tslib, but havent done anything about it yet (not sure if it can make bundle smaller or not yet)

In case of merging you are more than welcome to squash all the commits

codesandbox-ci · 2020-05-12T14:26:12Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 43c841d:

Sandbox	Source
amazing-chatelet-w3d0w	Configuration

ValentinH · 2020-05-12T14:30:45Z

Thanks for this PR.

However, using import cssStyles from './styles.css' is only possible in Webpack. I guess it works if you run the demo locally, however if you look at the codesandbox that was automatically created to point on your compiled version, it doesn't work.

stefanoruth · 2020-05-12T14:33:45Z

Yeah I can see the compiled code uses a dependency from node_modules in the compiled code which it shouldn't, i will take another look at this ;)

stefanoruth · 2020-05-12T14:35:33Z

But what do you think of letting the code inject at styletag with the styles next to the component it self?

ValentinH · 2020-05-12T18:02:16Z

But what do you think of letting the code inject at styletag with the styles next to the component it self?

I find it awkward to be honest, I have never seen this before.
I think that if we want to use plain CSS, we should ask the developer to import it explicitly.

Please see https://dev.to/stereobooster/what-is-the-best-way-to-distribute-styles-with-npm-package-m5f

stefanoruth · 2020-05-13T06:56:12Z

Yeah i agree it a bit awkward also why id added the option to disable it, the article you are linking to also suggest some other css-in-js libs which has alot smaller size fx, https://bundlephobia.com/result?p=linaria@1.3.3 Which we could implement instead of emotion, but that leads to some other complications with SSR, but yeah exposing a css file to import proberly is the best solution even tho it gives some more work to the user of the package.

Ps the sandbox works now

ValentinH · 2020-05-13T07:18:23Z

I didn't know linaria. If it works I'd be happy to use it in this library!

stefanoruth · 2020-05-13T09:29:49Z

Truth be told i have never heard of linaria before that article

ValentinH · 2020-05-13T09:41:35Z

bili.config.ts

@@ -8,10 +8,16 @@ const config: Config = {
    format: ['cjs', 'umd', 'umd-min', 'esm'],
    moduleName: 'ReactEasyCrop',
    sourceMap: true,
+    extractCSS: false,


could you explain a bit what these 2 new options are doing please ? 🙂

By default the built tool bili extract css imports to its own files and output then in the dist folder, this disabled that feature,

After disabling it, the bili rollup configuration is set to automaticly inject the style tag in the head of the dom, this postcss config disables that, we needed to disabled that feature else the rollup script would end up inside the bundle

This is the script that started to get bundled and the reason why the sandbox didnt works this first time: https://github.com/egoist/rollup-plugin-postcss#inject

ValentinH · 2020-05-13T09:42:29Z

examples/src/index.tsx

@@ -60,6 +60,7 @@ class App extends React.Component<{}, State> {
  render() {
    return (
      <div className="App">
+        <style>{styles}</style>


Why do we have to change the example? I think we should aim to keep the previous version working.

If I understand correctly, this change is not needed.

We actually dont have to change this, main reason was that i needed to change the webpack config to inline the css, and so this would fail because it also got inline, but i can create 2 different webpack rules for the src and example so we can keep the current behavoir and dont need to change the example ;)

ValentinH · 2020-05-13T09:43:19Z

Just tried the sandbox and it looks good to me, really cool stuff. I need more time to test this carefully and really understand the implications :).

I mainly want to read the build output :)

stefanoruth · 2020-05-13T13:23:06Z

That totally understandable, let me know if you want me to change anything ;)

ValentinH · 2020-05-13T19:46:22Z

@stefanoruth I've checked it locally and I like the approach. However, I would change one thing: I would inject the style tag in the <head> instead of in the middle of the body.

This can be done in componentDidMount() with:

const styleTag = document.createElement('style')
styleTag.setAttribute('type', 'text/css')
styleTag.innerHTML = cssStyles
document.head.appendChild(styleTag)

Also I don't think we need the disableInlineCss as without css the library cannot be used. I would release it like this without any new options. If there are some people willing to be able to manually inject the styles, this could be added later.

What do you think?

stefanoruth · 2020-05-14T06:35:22Z

Move it to head now with the code you posted, also removed the disableInlineCss 😄 but i still think it would be a good idear to include somekind of prop to this the autoinjection, and let people add it to their own css setup. Both in terms of people being able add the style to their caching setup and add it to their own css processors, to their can any the of the autoprefixes that they have configured in their setup

ValentinH · 2020-05-14T06:40:33Z

The more I think about it the more I agree :)

I quickly tried tweaking the config yesterday to also export the css as a plain file but didn't find a way so far. One possible approach would be to copy the src one to the dist folder with a simple bash command.

However, do you think we should create another export file without the inline CSS? This might be a too advanced optimisation for now so we could only start with just providing the CSS file and document how to use it manually.

stefanoruth · 2020-05-14T12:15:24Z

Hmm i have a few options right now that i am considering:

Option A: Only export a css file and document how to use it by importing it. (This is definitely a breaking change)

Option B: Keeping the inline plus exporting a css file with documentation, but adding a prop to disable the inline (Could not be a breaking change, but could be used as a step towards Option A or permanent) (Exporting could as you suggested just be bash copy/paste)

Option C: Only having the inline styles + a prop to disable it, and letting people just copy paste the styles from the src folder into their project.

I am most for Option A but that being a breaking change might leave option b as the better choice.

ValentinH · 2020-05-14T12:37:49Z

Thanks for detailing the options :)

I'm more in favour of option B for 2 reasons:

avoid breaking changes (even if I'll still bump a major version just in case)
not having to import the CSS manually is handy.

stefanoruth · 2020-05-14T13:47:08Z

Added option b now:

-Copied the css and rename the file to what ever format bili build system normally would have called a compiled css file.

Added a props called disableDefaultStyles to turn of the inject of styles in head, not sure about the naming of the props

Missing: some documentation of the prop and how to import the css file

ValentinH · 2020-05-14T14:25:18Z

src/index.tsx

@@ -46,6 +46,7 @@ type Props = {
  restrictPosition: boolean
  initialCroppedAreaPixels?: Area
  mediaProps: React.ImgHTMLAttributes<HTMLElement> | React.VideoHTMLAttributes<HTMLElement>
+  disableDefaultStyles?: boolean


I would suggest: disableAutomaticFileInjection

stefanoruth · 2020-05-15T06:29:40Z

Changed the name of the props + add some documentation to the readme file, it seems like my code editor change some more of the structure of the file, let me know it you want me to try and revert the formatting

ValentinH · 2020-05-15T06:39:09Z

I'm sorry but I did a mistake when suggesting the name 😬. I wanted to say disableAutomaticStylesInjection 😅

One thing I thought about yesterday evening is if injecting at the end of the head is not breaking the support of custom classnames passed via props. I need to check.

stefanoruth · 2020-05-15T06:48:54Z

Ill update it :), thats fair enough ;)

ValentinH · 2020-05-15T07:53:16Z

Thank you really much! I'm going to publish this as version 3.0.0 as soon as NPM is back: https://status.npmjs.org 😅

ValentinH · 2020-05-15T07:56:46Z

@allcontributors please add @stefanoruth for code and ideas

allcontributors · 2020-05-15T07:56:55Z

@ValentinH

I've put up a pull request to add @stefanoruth! 🎉

ValentinH · 2020-05-15T10:30:35Z

Release published: https://github.com/ricardo-ch/react-easy-crop/releases/tag/v3.0.0 🎉

stefanoruth added 4 commits May 12, 2020 16:15

Remove Emotion and inline css

d4687c9

Remove console.log

940f180

Remove style & css loader

7dc8f32

Make sure custom classes is applied last

82d06ca

ValentinH linked an issue May 12, 2020 that may be closed by this pull request

Removal of emotion #126

Closed

Remove the injectStyle function from node_modules

890a2ab

ValentinH reviewed May 13, 2020

View reviewed changes

Updated webpack config so we dont need to edit the example

76f50ce

Move css to head

fa582ed

Added Option B, with prop and copy css

87ec3a0

ValentinH reviewed May 14, 2020

View reviewed changes

ValentinH and others added 2 commits May 14, 2020 19:12

Merge branch 'master' into removal-of-emotion

dd62e97

Rename prop + add prop documentation

5625a9d

Change name of props disableAutomaticStylesInjection

258387f

Update README.md

43c841d

ValentinH merged commit 6d6f356 into ValentinH:master May 15, 2020

allcontributors bot mentioned this pull request May 15, 2020

docs: add stefanoruth as a contributor #131

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Emotion and add inline css #127

Remove Emotion and add inline css #127

stefanoruth commented May 12, 2020 •

edited

Loading

codesandbox-ci bot commented May 12, 2020 •

edited

Loading

ValentinH commented May 12, 2020

stefanoruth commented May 12, 2020 •

edited

Loading

stefanoruth commented May 12, 2020

ValentinH commented May 12, 2020

stefanoruth commented May 13, 2020 •

edited

Loading

ValentinH commented May 13, 2020

stefanoruth commented May 13, 2020

ValentinH May 13, 2020

stefanoruth May 13, 2020 •

edited

Loading

ValentinH May 13, 2020

stefanoruth May 13, 2020

ValentinH commented May 13, 2020

stefanoruth commented May 13, 2020

ValentinH commented May 13, 2020 •

edited

Loading

stefanoruth commented May 14, 2020

ValentinH commented May 14, 2020

stefanoruth commented May 14, 2020

ValentinH commented May 14, 2020

stefanoruth commented May 14, 2020

ValentinH May 14, 2020

stefanoruth commented May 15, 2020

ValentinH commented May 15, 2020 •

edited

Loading

stefanoruth commented May 15, 2020

ValentinH commented May 15, 2020

ValentinH commented May 15, 2020

allcontributors bot commented May 15, 2020

ValentinH commented May 15, 2020

Remove Emotion and add inline css #127

Remove Emotion and add inline css #127

Conversation

stefanoruth commented May 12, 2020 • edited Loading

codesandbox-ci bot commented May 12, 2020 • edited Loading

ValentinH commented May 12, 2020

stefanoruth commented May 12, 2020 • edited Loading

stefanoruth commented May 12, 2020

ValentinH commented May 12, 2020

stefanoruth commented May 13, 2020 • edited Loading

ValentinH commented May 13, 2020

stefanoruth commented May 13, 2020

ValentinH May 13, 2020

Choose a reason for hiding this comment

stefanoruth May 13, 2020 • edited Loading

Choose a reason for hiding this comment

ValentinH May 13, 2020

Choose a reason for hiding this comment

stefanoruth May 13, 2020

Choose a reason for hiding this comment

ValentinH commented May 13, 2020

stefanoruth commented May 13, 2020

ValentinH commented May 13, 2020 • edited Loading

stefanoruth commented May 14, 2020

ValentinH commented May 14, 2020

stefanoruth commented May 14, 2020

ValentinH commented May 14, 2020

stefanoruth commented May 14, 2020

ValentinH May 14, 2020

Choose a reason for hiding this comment

stefanoruth commented May 15, 2020

ValentinH commented May 15, 2020 • edited Loading

stefanoruth commented May 15, 2020

ValentinH commented May 15, 2020

ValentinH commented May 15, 2020

allcontributors bot commented May 15, 2020

ValentinH commented May 15, 2020

stefanoruth commented May 12, 2020 •

edited

Loading

codesandbox-ci bot commented May 12, 2020 •

edited

Loading

stefanoruth commented May 12, 2020 •

edited

Loading

stefanoruth commented May 13, 2020 •

edited

Loading

stefanoruth May 13, 2020 •

edited

Loading

ValentinH commented May 13, 2020 •

edited

Loading

ValentinH commented May 15, 2020 •

edited

Loading