-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Documentation: Updated TS documentation for re-using play functions #21517
Conversation
The current documentation introduces typescript errors when the rest of the context object is not passed to the re-used play functions.
… instead of destruct.
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.
These are great improvements! Thanks, and congratulations on your first contribution, @Foxhoundn! 🥳
If you don't mind, could you do a Find in the /docs
directory for play: async ({ canvasElement })
? I'm guessing there's a bunch more snippets around with this anti-pattern present.
Hi, there are yes, but those are not causing any issues since they only use the
to:
? |
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.
@Foxhoundn, thanks for taking the time and effort into this pull request and helping improve the documentation with this great find. Appreciate it 🙏 ! I left one small item that requires your attention before merging this. And I'm with you on this comment, as we shouldn't enforce passing down the entire context unless absolutely needed. Which is the reason you've pushed this pull request.
Let me know once you've addressed the changes so we can merge this.
Hope you have a great day.
Stay safe
@@ -34,12 +34,12 @@ export const SecondStory: Story = { | |||
}; | |||
|
|||
export const CombinedStories: Story = { | |||
play: async ({ canvasElement }) => { | |||
const canvas = within(canvasElement); | |||
play: async (context) => { |
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.
@Foxhoundn can you add a small comment in the example explaining why the user would use context
instead of the destructured canvasElement
, so that we can clarify to the readers? Also if you're able, can you also apply this pattern to this file, so that folks don't see one example with canvasElement
and another with context
Oh, that's a great point. No, I agree with you that they do not need changed. Thanks for the back-and-forth! |
Well to be honest - I do not know why the whole context is required, it's how the play function is typed. Thinking about it more and seeing that if you One reason for this would be when you want to reuse So @jonniebigodes, @kylegach, @yannbf and @IanVS - please let me know what you think :) |
@Foxhoundn, thanks for the follow-up and for bringing this to our attention. Once again, we're thankful for it 🙇 . And indeed, that's a rather interesting case we'll need to look into so that we don't lose track of this. Would you be ok with opening up an issue and mentioning this pull request, the last comment, and the Discord conversation? Please let me know when you've done it, and we'll go from there. |
Done! - #21573 |
The current documentation introduces typescript errors when the rest of the context object is not passed to the re-used play functions. Hi @IanVS & @jonniebigodes.
What I did
Added rest of the context object to the play functions
How to test
Follow the current documentation on TypeScript 4.9, you should see the following error (or a variation of it)
With this fix, the error should be gone.
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]