Open tay1orjones opened 4 months ago
Thank you for submitting a feature request. Your proposal is open and will soon be triaged by the Carbon team.
If your proposal is accepted and the Carbon team has bandwidth they will take on the issue, or else request you or other volunteers from the community to work on this issue.
Riffing today atop the context approach:
Multiple components recently gained support for
autoAlign
behavior. Oftentimes these components rely on one another, all the way down to a primitive, e.g.IconButton
➡️Tooltip
➡️Popover
While
autoAlign
is a single prop, there are multiple other props related to the positioning of floating elements that should be configurable by a consumer. One example is thealign
prop. We've had to add this prop to all these components and ensure they're passed down properly.This approach of passing props, sometimes called "prop drilling", is fine but there are some downsides:
<IconButton/>
.Goal
Provide a consumer a way to configure one/many popover-based elements in a single place, from anywhere within their react tree.
Solution options
The react docs suggest to start with prop drilling, then "extract components and pass JSX as children to them", before finally exploring using context.
1. Prop drilling
This is the existing approach.
2. Composition
Components such as IconButton could be updated to modify how they handle
children
. If a Tooltip is provided aschildren
, there could be special treatment for that element all the way down the tree.This gives consumers more direct control of the underlying popover-based elements.
This would need to work alongside the existing prop-drilling approach, so the prop API would not change. We could update all the documentation though to show this new approach as an option, maybe the "recommended" option.
2. Context
Expose a new context to consumers that can be configured anywhere in their react tree. This context would house all the popover-related props that a consumer would want to configure all at the same time -
align
,autoAlign
,autoAlignBoundary
, etc. Props likelabel
may not make sense to include in this context since consumers are likely going to need to configure content on a per-popover basis.This would require a new (
PopoverContext
?PopoverPlacementContext
?) context provider component for consumers to set the values:Consumer components that compose popover-based elements could also read from the context if they want to do conditional overrides for instance:
Components internal to the system would be updated to conditionally read from context while preferring local overrides
Overrides are possible via immediate prop usage. In this case below anything in the above context would be ignored. This also means to opt into this new feature you'd need to configure the context and remove explicit usages of
align
or other props.I think the values in context would only be used if there is not a specific prop already specified. We'd need to test this out because I'm sure we could find odd usage cases where a Tooltip "inherits" values that we wouldn't expect, or the prop override may not work properly.
I'm not sure of the value of
#2
and lean towards just going for#3
. It's similar to what we've done in other nest-able context providers likeLayer
,IdPrefix
,ClassPrefix
, andGrid
.