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

Fixed React Fiber deprecation of using ref with stateless function components #1551

Merged
merged 7 commits into from
Aug 10, 2017

Conversation

oriSomething
Copy link
Contributor

Issue: #1539

In short, React Fiber deprecated the ability to add ref to a stateless function component.

What I did

Converted LeftPanel and Shortcuts from stateless function components to React.Components.
It should be said that it seems the documentations of developing storybook slightly outdated and missing some information.
Another thing to mention is that I had to use eslint-disable for the components since according to some ESLint React plugin rule they shouldn't be a regular React.Component.

I hope I did an okay fix. Please, let me know.

How to test

  1. clone the repo with fix
  2. run npm install
  3. run npm link
  4. go to lib/ui and run there npm link
  5. go to app/react and over there do the following:
  6. run npm i react@next react-dom@next --only=dev
  7. run `npm link '@storybook/ui'
  8. run npm link

After the flowing you should create a React project using React Fiber.
Needs over there to set-up storybook and then run npm link '@storybook/react'.
The errors for issue #1539 should gone

Fix issue 1539

@oriSomething
Copy link
Contributor Author

It seems there is no relation to the code and the failing hooks. Can someone from the maintainers have a look?

@oriSomething
Copy link
Contributor Author

I get this in the page of ci/circleci: build_accept_deploy/deploy
screen shot 2017-08-02 at 10 44 47

@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #1551 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1551      +/-   ##
==========================================
- Coverage   21.34%   21.32%   -0.02%     
==========================================
  Files         244      244              
  Lines        5378     5378              
  Branches      658      657       -1     
==========================================
- Hits         1148     1147       -1     
- Misses       3730     3744      +14     
+ Partials      500      487      -13
Impacted Files Coverage Δ
lib/ui/src/modules/ui/components/shortcuts_help.js 40.9% <0%> (-4.55%) ⬇️
...b/ui/src/modules/ui/components/left_panel/index.js 100% <100%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 22.36% <0%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 28.04% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/left_panel/header.js 28.57% <0%> (ø) ⬆️
... and 23 more

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 df6b304...1a3475a. Read the comment docs.

@oriSomething
Copy link
Contributor Author

From what I've checked I've finished the PR a week ago, and since then, I only rebase from master.
Can anyone from the members of the repo tell me if anything else needs to be done? I would be happy if this PR will be merged since I already use React 16

@Hypnosphi Hypnosphi self-assigned this Aug 9, 2017
@Hypnosphi Hypnosphi self-requested a review August 9, 2017 20:56
@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 9, 2017

That's a shame you had no response for so long =(

As for reproduction steps, does it work with npm run bootstrap instead of manual linking?

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

Ok, looks like it's ShortcutsHelp that needed to be converted to class. I fixed this. Thanks a lot anyway!

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.

2 participants