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 storybook #27

Merged
merged 8 commits into from
Aug 3, 2021
Merged

Conversation

coling
Copy link
Contributor

@coling coling commented Jul 28, 2021

This is an attempt to update story book to the latest version. Comments welcome.

Description

I updated my dev environment to npm 7 and ran into problems with storybook build so attempted to fix this up. I suspect the changes could all be avoided just by using npm install --legacy-peer-deps which I ultimately had to use anyway but I figure these changes will be needed eventually anyway.

Sadly, there are a bunch of problems that still remain (namely that this project requires react 17 and storybook is sadly still stuck requiring react 16 due to it's use of @reach/router. There are plans afoot to fix this, but in the mean time I still had to use npm install --legacy-peer-deps to get a working build.

In Storybook 6, addon-knobs has been deprecated but it still works for now. It should ultimately be migrated to addon-controls

Motivation and Context

I wanted this to work with latest npm. Failed and still had to use --legacy-peer-deps but upgrade still worked as a result.

How Has This Been Tested?

I've tested this with npm, not used yarn. I was able to build the storybook and run it directly. I've not tried anything else related to release or building of the project.

Other than that, this was latest npm/node packages from npmsource DNF repository running in a CentOS 8 container.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Development/Build/Documentation change

@coling
Copy link
Contributor Author

coling commented Jul 28, 2021

Just as a general comment, the last pull request I made with multiple commits was ultimately squashed and committed as one commit rather than merged. Don't know if this is a standard convention but it was quite confusing to see this happen as the detail in the individual commits was lost. IMO merging the branch (possibly with --no-ff) maintains a better history but I'm sure this is generally down to personal preference :-)

Hope you're well? Cheers!

Copy link
Contributor Author

@coling coling left a comment

Choose a reason for hiding this comment

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

For what it's worth, I found out why dayjs().add() was an issue. It's due to an upstream bug which although marked as closed, it's just been marked as a duplicate. This workaround here seems like a valid approach for now.

@@ -21,16 +21,12 @@
"rangePicker",
"datePicker"
],
"peerDependencies": {
"react": "^17.0.1",
Copy link
Owner

Choose a reason for hiding this comment

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

Please build this package and link it and use it to another react project , you will be get some error like this, I think we should remain peerDependencies

Screen Shot 1400-05-07 at 21 25 10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh perhaps I misunderstood the docs... They suggested adding dev deps which I incorrectly assumed meant peerDeps weren't needed. Keeping them as both should work fine I guess... Will have a play later.

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, Sorry Maybe I was wrong, Please have a look at this;

I had double react version problem at the beginning of this package, when I was using react in dev deps, I search a lot then some guys suggest me to using in the pear deps. after the new version, it has not any problem after install it in the project.

@samsam-ahmadi
Copy link
Owner

Hey Thanks for you PR.

I'm feeling good, Thanks.
Sorry about the squashed your last PR, I was thinking maybe it would be clean the history but i was wrong.
I will merge these PRs as before.
Just one comment about moving the react to the devdepenceny It's make some error when we want to link the component in local and use it in another project.
Would you please double check ?

@samsam-ahmadi samsam-ahmadi merged commit 2f4e576 into samsam-ahmadi:master Aug 3, 2021
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.

Upgrade @reach/router to react-router@6
2 participants