Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

feat(Storybook): extend storybook for developer focused demo - #315 #382

Merged

Conversation

98lenvi
Copy link
Contributor

@98lenvi 98lenvi commented Apr 13, 2020

Issue #315 #376

Complete support added for Cicero-UI components in Storybook.

Preview available here

Changes

  • Extended webpackconfig of storybook's default configuartion
    • Added sass loader
    • Excluding jsdom
    • Added config to ignore core Node APIs (fs,net,tls) during build.
  • Moved storybook and related dependencies to dev-dependencies.
  • Added netlify.toml to netlify to storybook build

Previously added, now removed-

  • Added CleanWebpackPlugin()
  • Added HtmlWebpackPlugin
  • Added MiniCssExtractPlugin

Flags

Need to add the following (DONE)

  • ContractEditor
  • TemplateLibrary
  • ErrorLogger
  • Navigation
  • Demo

Related Issues

98lenvi added 2 commits April 14, 2020 00:46
Signed-off-by: lenvi <lenvin@oykuapp.com>
Signed-off-by: lenvi <lenvin@oykuapp.com>
98lenvi added 5 commits April 14, 2020 15:45
Signed-off-by: lenvi <lenvin@oykuapp.com>
Signed-off-by: lenvi <lenvin@oykuapp.com>
Signed-off-by: lenvi <lenvin@oykuapp.com>
Signed-off-by: lenvi <lenvin@oykuapp.com>
Signed-off-by: lenvi <lenvin@oykuapp.com>
@98lenvi 98lenvi changed the title [WIP] feat(Storybook): extend storybook for developer focused demo - #315 feat(Storybook): extend storybook for developer focused demo - #315 Apr 14, 2020
@98lenvi
Copy link
Contributor Author

98lenvi commented Apr 14, 2020

@irmerk, I hope it's complete now. Can you please go through it and let me know if anything needs to be added / modified

Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

Added a few comments. Also, is there a way to see the netlify build of storybook for this PR?

template: "demo/src/index.html",
mountId: "demo",
title: "YES"
Copy link
Member

Choose a reason for hiding this comment

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

Can we make the title something like "Cicero UI Components Demo"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, Will do it right away!

@@ -203,3 +203,6 @@ function Demo() {
}

render(<Demo/>, document.querySelector('#root'));

//For usage with Storybook (temporary)
Copy link
Member

Choose a reason for hiding this comment

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

What does this comment mean? Is it temporary because we will make a new "Demo" component for storybook instead of using the existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had added lines 207 & 208 in this commit. It was added to export the Demo component to be used with storybook for testing.

I have now reverted the changes (i.e. I have now removed line 207 & 208) now as Demo is no longer needed in Storybook.

98lenvi added 2 commits April 15, 2020 16:26
Signed-off-by: lenvi <lenvin@oykuapp.com>
@98lenvi
Copy link
Contributor Author

98lenvi commented Apr 15, 2020

Added a few comments. Also, is there a way to see the netlify build of storybook for this PR?

@DianaLease I will be adding a netlify.toml file for that in a commit.

98lenvi added 2 commits April 15, 2020 16:37
…build - i315

Signed-off-by: lenvi <lenvin@oykuapp.com>
@jolanglinais
Copy link
Member

This is super exciting @98lenvi, great work. Looking into the code now.

@jolanglinais
Copy link
Member

In the Notes tab for most of these, we'll want to make sure to format some things to make it consistent with the .md format here in the GitHub repo. Example is the main Demo which has the README.md from the main repo here, and the table for Ecosystem > Core libraries is a bit off.

@dselman
Copy link
Collaborator

dselman commented Apr 15, 2020

In the Notes tab for most of these, we'll want to make sure to format some things to make it consistent with the .md format here in the GitHub repo. Example is the main Demo which has the README.md from the main repo here, and the table for Ecosystem > Core libraries is a bit off.

Let's try to avoid duplication as much as possible to keep things maintainable. E.g. link to the existing README.md.

Copy link
Member

@jeromesimeon jeromesimeon left a comment

Choose a reason for hiding this comment

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

LGTM. Question about the netlify configuration.

@@ -0,0 +1,43 @@
# Settings in the [build] context are global and are applied to all contexts
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a netlify configuration? We don't do that on any of our projects -- should it be instead done within netlify itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be done in Netlify instead.

Signed-off-by: lenvi <lenvin@oykuapp.com>
@98lenvi
Copy link
Contributor Author

98lenvi commented Apr 15, 2020

In the Notes tab for most of these, we'll want to make sure to format some things to make it consistent with the .md format here in the GitHub repo. Example is the main Demo which has the README.md from the main repo here, and the table for Ecosystem > Core libraries is a bit off.

I will look into that, can't understand why the formatting looks off. @irmerk

Let's try to avoid duplication as much as possible to keep things maintainable. E.g. link to the existing README.md.

Done in the latest commit. @dselman

@jolanglinais
Copy link
Member

I'm thinking we get this in and start iterating on it to stop blocking this PR. Just need to make sure local development of ContractEditor still works as we expect.

@98lenvi
Copy link
Contributor Author

98lenvi commented Apr 16, 2020

@irmerk Sure! I just need to work on one more commit to add knobs to ContractEditor story ( I missed the lockText and readOnly props)

Signed-off-by: Lenvin Gonsalves <lenvin@oykuapp.com>
@98lenvi
Copy link
Contributor Author

98lenvi commented Apr 16, 2020

@irmerk, I made the changes, i.e. add readOnly knob. For some reason, lockText doesn't work via a knob. Even if I declare it as a state variable and change its value, there is no effect on the ContractEditor.

@DianaLease
Copy link
Member

DianaLease commented Apr 16, 2020

@irmerk, I made the changes, i.e. add readOnly knob. For some reason, lockText doesn't work via a knob. Even if I declare it as a state variable and change its value, there is no effect on the ContractEditor.

This sounds similar to an issue we had with the old version of slate where prop changes wouldn't re-render the editor. It was working for me with the new slate when we had a toggle switch though that updated the value of lockText. Let me try again locally with a toggle and reconfirm.

EDIT: @98lenvi looks like this is a separate issue. I'll look into it!

@jolanglinais
Copy link
Member

Confirming this works for local development (npm run storybook instead of npm run start)

Copy link
Member

@jolanglinais jolanglinais left a comment

Choose a reason for hiding this comment

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

I think this is a good first step, second 👀 @DianaLease?

Copy link
Member

@DianaLease DianaLease left a comment

Choose a reason for hiding this comment

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

Awesome!

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

Successfully merging this pull request may close these issues.

5 participants