feat(components): Improve Composable Menu Types #2782
Draft
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.
Motivations
minor improvement I wanted to hop on before we have to deal with backwards compatibility via people using the props in their current state
destructive
having it as a boolean is not great for a few reasons that Eric pointed out on the PR. 1. can't extend it easily in the future if say we have a "success" or "informational" equivalent and 2. if we ever did have 2 things that want to style, it gets tough to validate this
<MenuItem destructive informational>
of course it's a bit silly to do that, and that somewhat solves itself if you just don't do that, but there's also an easy, established pattern to avoid this viavariation
that we make use of pretty often. so now it'svariation: "destructive" | "informational"
and you choose exactly one, and it's easy to add to in the future.then a minor, non functional change the the type on
children
this one I'm a bit less sure about. I realized that in the future, we don't want that wrappingdiv
as a child ofAriaPressable
but when it's justchildren
directly, the types don't like this because we have it asReactNode
and Pressable is a bit more particular about what it accepts.functionally this will have no difference now. it's just if we remove the div, the types are still happy - so it becomes less prone to changes later. the only thing I'm not sure about is that this is actually not required in one scenario
if we make our Button a wrapper around AriaButton, then you don't even need Pressable at all, and then the type I'm adding now isn't required.
it's possible that we wait until we're certain. while yes, changing the type is technically breaking and the longer we wait the more risk we have that someone violates it, realistically no one should be putting in non interactive elements as the Trigger, and that's the only case where it might get upset.
Changes
Added
Changed
Deprecated
Removed
Fixed
Security
Testing
Changes can be
tested via Pre-release
In Atlantis we use Github's built in pull request reviews.