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

Upgrade [email protected] #4925

Closed
wants to merge 3 commits into from
Closed

Upgrade [email protected] #4925

wants to merge 3 commits into from

Conversation

lifeiscontent
Copy link
Member

Issue:

What I did

upgraded all packages to use latest version of emotion

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@lifeiscontent
Copy link
Member Author

lifeiscontent commented Dec 5, 2018

not sure why the build is failing.

@igor-dv
Copy link
Member

igor-dv commented Dec 5, 2018

#4525 related

@codecov
Copy link

codecov bot commented Dec 5, 2018

Codecov Report

Merging #4925 into next will not change coverage.
The diff coverage is 40%.

Impacted file tree graph

@@           Coverage Diff           @@
##             next    #4925   +/-   ##
=======================================
  Coverage   35.02%   35.02%           
=======================================
  Files         566      566           
  Lines        7004     7004           
  Branches      937      937           
=======================================
  Hits         2453     2453           
  Misses       4055     4055           
  Partials      496      496
Impacted Files Coverage Δ
addons/knobs/src/components/types/Color.js 24% <ø> (ø) ⬆️
lib/ui/src/modules/ui/components/shortcuts_help.js 41.66% <ø> (ø) ⬆️
lib/ui/scripts/createDlls.js 0% <ø> (ø) ⬆️
lib/components/src/layout/index.js 100% <ø> (ø) ⬆️
lib/components/src/layout/desktop.js 74.13% <ø> (ø) ⬆️
...ddons/actions/src/components/ActionLogger/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/search_box.js 86.84% <0%> (ø) ⬆️
...ents/stories_panel/stories_tree/tree_decorators.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a06253...b8be2f4. Read the comment docs.

@@ -238,7 +238,7 @@ storiesOf('Button', module)

We use [emotion](https://emotion.sh) for styling, AND we provide a theme which can be set by the user!

We highly recommend you also use emotion to style your components for storybook, but it's not a requirement. You can use inline styles or another css-in-js lib. You can receive the theme as a prop by using the `withCSSContext` hoc from emotion. [Read more about theming](/configurations/theming).
We highly recommend you also use emotion to style your components for storybook, but it's not a requirement. You can use inline styles or another css-in-js lib. You can receive the theme as a prop by using the `withTheme` hoc from emotion. [Read more about theming](/configurations/theming).
Copy link
Member

Choose a reason for hiding this comment

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

I have to say, this looks like a breaking change...

Copy link
Member

Choose a reason for hiding this comment

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

@ndelangen I think we need to have some abstraction of the ThemeProvider at our side

@lifeiscontent
Copy link
Member Author

@igor-dv anything I can do on my end here or should I just wait till you guys solve some internal issues?

@igor-dv
Copy link
Member

igor-dv commented Dec 5, 2018

Let's see what @ndelangen thinks about #4925 (comment)

@lifeiscontent
Copy link
Member Author

Cc @igor-dv think you could ping @ndelangen? Would love this to get merged soon

@igor-dv
Copy link
Member

igor-dv commented Dec 10, 2018

Yes, will DM him.

@ndelangen
Copy link
Member

Hey @lifeiscontent thank you for this PR.

I want to merge this, but it does have something of a breaking change in it.
I fear releasing it quickly in a minor will not go down well.

The next major release is scheduled around the 15th of January.

What do you think we should do?

@ndelangen ndelangen self-assigned this Dec 10, 2018
@ndelangen
Copy link
Member

We can merge this into the overhaul ui branch today, but that's not usable by users yet.

@lifeiscontent
Copy link
Member Author

lifeiscontent commented Dec 11, 2018

@ndelangen I'm okay with waiting for it, I've currently just disabled all the plugins that break emotion 10. so no knobs for me at the moment, but storybook itself still works.

I actually haven't tested if the code actually fixes the issues I'm seeing, is there a way to easily test this code locally within my own project to confirm?

@ndelangen
Copy link
Member

I merged this (without issue) into #4086 see 2fae294

@ndelangen ndelangen closed this Dec 14, 2018
@igor-dv
Copy link
Member

igor-dv commented Dec 25, 2018

@ndelangen let's maybe merge this to v5 branch as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants