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

Feature/update storybook 5 3 #1867

Closed
wants to merge 14 commits into from
Closed

Conversation

raphaellueckl
Copy link
Contributor

@raphaellueckl raphaellueckl commented Jul 9, 2020

Fixes #1774

My changes are according to the most recent documentation. We use the very newest versions here. Feel free to adapt, but MAKE SURE that the following stuff works on IE11, otherwise we cannot run it on IE11.

It was a real struggle to get to this point. If you see minor issues, I HIGHLY advise you to merge it first and change the smaller stuff afterwards. If you think there are major issues, please take over the PR and add your ideas.

This project was created with the Storybook-Cli. :)

  • npm run test
  • npm run build-storybook && cd storybook-static && npx http-server -> Open IE11 on http://localhost:8080/
  • npm start -> Open IE11 on http://localhost:6006/
  • Verify that the addons work at all times (Keyboard Shortcut: D)
  • Make sure that assets like the axa logo and the favicons are displayed in all browsers.

@@ -1,4 +1,4 @@
import '@axa-ch/patterns-library-polyfill';
import './polyfills-ie11.js';
Copy link
Contributor

Choose a reason for hiding this comment

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

@raphaellueckl why this?

Copy link
Contributor

Choose a reason for hiding this comment

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

i mean, why not keep our ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We keep our ones, but "array.split" or something was not supported. But feel free to reduce them. That part caused a lot of work, but in the end, I never fully optimized it, now that you mention it. Good point.

BUT "our ones" are still there, just within that file. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have splitted that polyfill files into 2 imports:

  • the additional polyfills for storybook 5.3
  • our good old PL polyfills for our components

danyball
danyball previously approved these changes Jul 9, 2020
@danyball danyball self-assigned this Jul 10, 2020
@danyball
Copy link
Contributor

danyball commented Jul 10, 2020

JUST with npm start!!

We have 1 error on the console. But everything works. And its just on the first component you are clicking on.

It seems that this is a open storybook-bug:
storybookjs/storybook#10204

The error is gone if we remove changelog and codepreview addons.

@danyball
Copy link
Contributor

danyball commented Jul 10, 2020

JUST with npm start!!

We have a second warning - but it is not reproducable:
Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by React and is not a top-level container. Instead, have the parent component update its state and rerender in order to remove this component.

@danyball
Copy link
Contributor

danyball commented Jul 14, 2020

I suggest to update directly to the new SB 6, which will be releases at the end of july (storybookjs/storybook#9311). Because there is a problem at a SB internal file with the 5.3.

Have installed the 6-RC and it works with just our PL polyfills. No extra pf needed. We just have to some extra work on our SB plugins. There are breaking changes on how to load them. But the showcases are working:
image

We can go these 2 ways:

  1. Merge this PR with SB5.3 and the extra polyfills. After upgrading to 6 remove the extra polyfills.
  2. Keep the branch of this PR as a basis for SB6 upgrade.

What do you think @noahliechti @LucaMele ?

@01231
Copy link
Contributor

01231 commented Jul 14, 2020

I would choose 1. because we have some tasks for the new storybook/welcomepage design and I think the design tool integration which storybook provides with version 5.3 would be helpful for these tasks.

@LucaMele
Copy link
Contributor

@danyball I would wait to the one that works without polyfill hacks. End of July is only in 2 weeks and everyone is on holiday anyway @noahliechti so i guess it wont hurt the designer if they have to wait 2 weeks in holiday times for a cleaner setup

Added some comments for a better upgrade to 6.x
@danyball
Copy link
Contributor

We decided to not merge this PR (see comments). All checks have passed on this PR right now. So, the branch is ready to upgrade to 6.X.

We are closing the PR but do not delete the branch.

@danyball danyball closed this Jul 14, 2020
@danyball danyball deleted the feature/update-storybook-5-3 branch August 28, 2020 07:06
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.

[Child of #1773] Update Storybook to version 6 [Due Date: 03.08.20]
4 participants