-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
onPositionChanged = () => {}, | ||
onFlipped = () => {}, | ||
onPositionChanged, | ||
onFlipped, |
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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' }); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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? 😄
There was a problem hiding this comment.
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.
Pull request checklist
Please check if your PR fulfills the following requirements:
yarn build
) was run locally and any changes were pushedyarn lint
) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
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:
yarn storybook
to kick the local storybook serverDoes this introduce a breaking change?
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