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

Update emotion dependencies #124

Merged
merged 7 commits into from
Jun 27, 2019

Conversation

michaeljaltamirano
Copy link
Contributor

@michaeljaltamirano michaeljaltamirano commented Jun 27, 2019

What & Why

  1. emotion is the framework-agnostic version of the library. The Global component in @emotion/core achieves the same functionality as injectGlobal, so I've replaced that functionality and removed emotion as a dependency.
    • This will also fix the missing dependency issue in our main app.
  2. I've upgraded all emotion dependencies to 10.0.14. They are all updated in tandem so all emotion updates should occur simultaneously.

Other

We log the following warning in development (from our tests for button and roundButton:

      Warning: Received `false` for a non-boolean attribute `loading`.
      
      If you want to write it to the DOM, pass a string instead: loading="false" or loading={value.toString()}.
      
      If you used to conditionally omit it with loading={condition && value}, pass loading={condition ? value : undefined} instead.
          in div (created by Context.Consumer)
          in Loader (created by Context.Consumer)
          in button (created by Context.Consumer)
          in div (created by Context.Consumer)
          in RoundButton (created by WrapperComponent)
          in WrapperComponent

I may or may not create a follow-up PR shortly to update this, depending on how big the diff would be in our other repos.

@snags88 snags88 temporarily deployed to curology-radiance-pr-124 June 27, 2019 18:51 Inactive
@snags88
Copy link
Contributor

snags88 commented Jun 27, 2019

sweet, yeah we should deprecate loading and use isLoading. Probably need to support both for a minor update and then remove loading in the next major

Copy link
Contributor

@daigof daigof left a comment

Choose a reason for hiding this comment

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

looks good

@michaeljaltamirano
Copy link
Contributor Author

Currently investigating why the styles that I was seeing locally are: 1. Not on review app, but, more importantly, 2. Not appearing locally either 😅

@michaeljaltamirano
Copy link
Contributor Author

michaeljaltamirano commented Jun 27, 2019

I removed optionConfig thinking it was an unused import which broke the changes. I decided to move the optionConfig code into config in bf0efee to more closely match Storybook config documentation. I seems simpler than having multiple files in this case since we've added so much more global configuration (since I joined! 😅)

@michaeljaltamirano michaeljaltamirano merged commit c2f9631 into master Jun 27, 2019
@michaeljaltamirano michaeljaltamirano deleted the chore/update-emotion-dependencies branch June 27, 2019 22:03
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.

3 participants