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

feat: integrates storybook #125

Closed
wants to merge 3 commits into from
Closed

feat: integrates storybook #125

wants to merge 3 commits into from

Conversation

guilhermevrs
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (yarn build) was run locally and any changes were pushed
  • Lint (yarn lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

No visual documentation for the documents

Issue Number: Closes #121

What is the new behavior?

This PR integrates storybook with examples on the BaseCard component

A couple of changes:

  • In order to correct have addons-actions to work, no default value for callback parameters
  • Run yarn storybook to kick the local storybook server
  • Fixed minor issues with the eslint
  • No coverage for stories

Does this introduce a breaking change?

  • Yes
  • No

As described above, in order to have addons-actions to work, callback parameters should not have default values

Other information

In order to restain the size of this PR, I have migrated only BaseCard with examples. We would need to open another story to migrate the other ones.
Also, we would need as well to integrate a way of publishing the storybook result (like with Chromatic)

A picture tells a thousand words

image

@guilhermevrs guilhermevrs self-assigned this Dec 17, 2020
onPositionChanged = () => {},
onFlipped = () => {},
onPositionChanged,
onFlipped,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you have to change this? If you need to change it then maybe we should also set the property as not optional 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, with the default value for callbacks, Storybook actions (the panel that logs on event activation) seems to not work - it's on the PR description

The events should still to be optional IMO. Why exactly do you want to make them not optional?

backFace: <div>Back face</div>,
};

export default defaultStoryMetaFactory({ title: 'Pure' });
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this file, or do we? What is it doing exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a story showing we can use BaseCard with whatever you want as face (so if you want to use another type of card).
Doesn't make sense?

I advise you to run locally to see the stories running to get a better feeling of what is doing

@@ -1 +1,2 @@
/* istanbul ignore file */
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? We should rather configure jest to exclude index.tsx files in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I was wondering if we want to remove all the index.tsx files from coverage or not...
table slices for example have all the logic on the index.tsx.

Shall we move it then and remove all the index.tsx from the coverage?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would suggest to move it and remove all index from coverage. We can do it in another PR 😄

@@ -10,6 +10,7 @@
"moduleResolution": "node",
"allowSyntheticDefaultImports": true,
"esModuleInterop": true,
"strictBindCallApply": true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain me why you needed to add this option? 😄

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's to ensure that .bind({}) for functions enforce the typings.

Example here: https://github.com/drunkoders/cards/pull/125/files#diff-5a75b3b5d5cff2d3c58ea81dd1b63d9c86952e87c9575cd67352c3dfa1472659R5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Cards-ui
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Storybook integration
2 participants