Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Try menu appear animation & start work on handbook #13617
Try menu appear animation & start work on handbook #13617
Changes from 22 commits
dacfd32
1d1f164
5f17198
b028d51
bf10dff
ce7d7f1
a883d30
df62716
6842659
e26b700
e62c739
5415930
4ad8653
491aab5
0e5038e
1e52573
520503b
88a57f9
a2de8fc
d9e3601
1e9976a
58470a3
432c0e1
91b7144
8f9ef37
328b652
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When
children
isn't a function it will explode at the moment.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 think it's fine, it's a requirement for the component. it's like saying the
options
is an object and if you pass a number it will explode.I do see the specificity in JSX about the
children
prop but ultimately I think we can just consider it a requirement to be a function.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 think it's a conceptual issue with render props :)
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.
Is there a reason that
options
ought to be a separate object, vs. just having its own properties be props of the component itself?In other words, is...
...necessarily any better than:
?
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.
My thinking at the time is that "options" are type-specific, their format change for each animation type.
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.
Okay, thanks for clarifying. I'm not sure it would necessarily be wrong to still flatten these as top-level props, even if each
type
had their own handling and own set of options, but the rationale you give is sensible.It may have been misleading to me because in the current implementation, the only option is
origin
, and it is consistent for both theappear
andslide-in
types, though I suppose that's more a reflection of its current state, and these may be expanded at some point in the future.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.
Should it exit early when children isn't a function?
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.
Same as I was saying above, we don't guard about the other props. I'm hesitant to add a special case for children.