docs: add default stories for IDE components #2398
Conversation
There was a problem hiding this comment.
Excellent work 👍 You've got me thinking about lots of things and I've added many comments with things that we can improve in the future.
The biggest thing is how these components interact with Redux. We have two approaches here to make these stories make sense:
-
Each component exports a connected version which gets the state from Redux and an unconnected version which accepts props. We would use the unconnected version for Storybook. This is easy for the old components which use
connect. But for the ones that I've rewritten, likeTimer, I've generally been making it so that the component takes no props and accesses Redux internally. -
We pass the
argsof the story into a Redux provider so that we can specify the correct state. This requires setup and more work on the Storybook side, but no changes to the components which is a pro. But it also requires the story to know the structure of the Redux state, ie. thatprojectSavedTimeshould go instate.project.updatedAt, and that is a con to me because Redux should be a black box.
| url: 'https://p5js.org/assets/img/p5js.svg', | ||
| name: 'P5 Logo' | ||
| } | ||
| }; |
There was a problem hiding this comment.
We can add stories for audio and video files (can be done later).
|
|
||
| export const Default = { | ||
| args: { | ||
| totalSize: 123 |
There was a problem hiding this comment.
This story shows up with a control for the totalSize, but changing it doesn't do anything because the default export gets the size from Redux.
Once we get the styles working and once we get this using the args then it might be cool to have stories for empty vs. full vs. somewhere in the middle.
| value: 'an@email.com', | ||
| label: 'Example' | ||
| } | ||
| }; |
There was a problem hiding this comment.
We aren't seeing the pencil icon here due to an issue with svgr. I think it's removing the viewbox. Just FYI, we can fix that later.
| component: NewFileForm | ||
| }; | ||
|
|
||
| export const Default = {}; |
There was a problem hiding this comment.
The NewFileForm is only used inside of the NewFileModal so it might be redundant to have stories for both. (the same for the NerFolderForm)
TBH we could consider combing the two component files as there's not much left in the NewFileModal after we made a common Modal component.
There was a problem hiding this comment.
I like the idea of having fewer Modals and reusing the generic one. I think thats capturing the layout primitives
There was a problem hiding this comment.
I've removed NerFolderForm
| sketches: [ | ||
| { | ||
| id: '1', | ||
| name: 'Play Sketch', | ||
| createdAt: Date.now().toString(), | ||
| updatedAt: Date.now().toString() | ||
| } | ||
| ] |
There was a problem hiding this comment.
I was wondering why these sketches don't show up in the list. Again, it's because the component ignores this prop and gets the actual value from Redux.
There was a problem hiding this comment.
Ahhh! I was wondering that myself but was unsure.
lindapaiste
added
the
Area: Documentation
…b-editor into docs-add-default-stories
|
@lindapaiste is there anything here you'd like me to action? I think the comments are follow up items. |
Changes:
Adds default stories to IDE components. At least to the ones that make sense. There are a few like Toast, Timer and EditorAccessibility which is either hidden or needs some redux magic set up to trigger output.
There is more that could be done for each story to offer any other variations. But for a first pass I think it's good to get a simple story. I think would ease development for things like file upload or copyable field as they are now surfaced as an isolated story.
I have verified that this pull request:
npm run lint)npm run test)developbranch.Fixes #123