When passing props like the leadingIcon and trailingIcon props on a button, it's not uncommon to pass something that relates to props from the containing function.
Currently, to do this one needs to wrap this in a function
<Button leadingIcon={() => getAnIconBasedOnSomething(props)} />This leads to Button needing to render more often than necessary
we could always memoize this, of course
<Button leadingIcon={useCallback(() => getAnIconBasedOnSomething(prop1, prop2), [prop1, prop2])} />But react already has a built in mechanism for dealing with this, components.
<Button leadingIcon={<IconBasedOnSomething prop={prop} />} />This manages these icons, and similar (I assume in other components) in an internal way, similar to children by default, allows the parent context to control what gets rendered, rather than internalizing that to the button, and avoids the need to either bloat code by memoizing what are effectively inline components or handling these inefficiently by default
|
return ( |
|
<StyledButton as={Component} sx={sxStyles} {...props} ref={forwardedRef}> |
|
{LeadingIcon && ( |
|
<Box as="span" data-component="leadingIcon" sx={iconWrapStyles}> |
|
<LeadingIcon /> |
|
</Box> |
|
)} |
|
{children && <span data-component="text">{children}</span>} |
|
{TrailingIcon && ( |
|
<Box as="span" data-component="trailingIcon" sx={trailingIconStyles}> |
|
<TrailingIcon /> |
|
</Box> |
|
)} |
|
</StyledButton> |
could be minimally changed to support that
const ButtonBase = forwardRef<HTMLElement, ButtonProps>(
({children, as: Component = 'button', sx: sxProp = defaultSxProp, ...props}, forwardedRef): JSX.Element => {
const {leadingIcon, trailingIcon, variant = 'default', size = 'medium'} = props
const {theme} = useTheme()
const baseStyles = useMemo(() => {
return merge.all([getButtonStyles(theme), getSizeStyles(size, variant, false), getVariantStyles(variant, theme)])
}, [theme, size, variant])
const sxStyles = useMemo(() => {
return merge(baseStyles, sxProp as SxProp)
}, [baseStyles, sxProp])
return (
<StyledButton as={Component} sx={sxStyles} {...props} ref={forwardedRef}>
{leadingIcon && (
<Box as="span" data-component="leadingIcon" sx={iconWrapStyles}>
{leadingIcon}
</Box>
)}
{children && <span data-component="text">{children}</span>}
{trailingIcon && (
<Box as="span" data-component="trailingIcon" sx={trailingIconStyles}>
{trailingIcon}
</Box>
)}
</StyledButton>
)
}
) as PolymorphicForwardRefComponent<'button' | 'a', ButtonProps>with no loss internally and more power for consumers to render what/how they need to
this would make the simple case slightly more complex - by default
<Button leadingIcon={LeadingIcon} />we could choose to handle this internally, but checking whether react-is.isComponentTypes (any of them), and handling them separately. I don't think we should, because this is a moving target of possibility - prone to error, and the difference is minimal to consumers who are used to writing JSX when dealing with components
Components rendered this way could be memo forwardRef lazy or more, none of which are matches for the FunctionComponent interface currently, and all implement a standard that considers them React.ReactNodes
When passing props like the
leadingIconandtrailingIconprops on a button, it's not uncommon to pass something that relates to props from the containing function.Currently, to do this one needs to wrap this in a function
This leads to
Buttonneeding to render more often than necessarywe could always memoize this, of course
But react already has a built in mechanism for dealing with this, components.
This manages these icons, and similar (I assume in other components) in an internal way, similar to
childrenby default, allows the parent context to control what gets rendered, rather than internalizing that to the button, and avoids the need to either bloat code by memoizing what are effectively inline components or handling these inefficiently by defaultreact/src/Button/ButtonBase.tsx
Lines 29 to 42 in c3eedb2
could be minimally changed to support that
with no loss internally and more power for consumers to render what/how they need to
this would make the simple case slightly more complex - by default
we could choose to handle this internally, but checking whether react-is.isComponentTypes (any of them), and handling them separately. I don't think we should, because this is a moving target of possibility - prone to error, and the difference is minimal to consumers who are used to writing JSX when dealing with components
Components rendered this way could be
memoforwardReflazyor more, none of which are matches for the FunctionComponent interface currently, and all implement a standard that considers them React.ReactNodes