chore: replace deprecated retrieveComponentStyles with componentStyles utility
#1989
Conversation
| @@ -18,5 +18,5 @@ export const StyledInput = styled(Input as any).attrs({ | |||
| })` | |||
| text-align: center; | |||
|
|
|||
| ${props => retrieveComponentStyles(COMPONENT_ID, props)}; | |||
| ${componentStyles}; | |||
There was a problem hiding this comment.
Food for thought...
At first glance, one could assume that the componentId received by componentStyles is colorpickers.colorpicker_input.
Sadly, that's incorrect as this story's DOM shows. 😞
The data-garden-id is in fact coming from the component returned by Input: StyledTextInput.
This PR explains why and what pattern works with great detail.
For the intended behavior, we should explicitly define componentId.
| ${componentStyles}; | |
| ${(props) => componentStyles({ ...props, componentId: COMPONENT_ID })}; |
However, unless our users found the colorpickers.colorpicker_input component id by exploring the source code, it's unlikely they knew the ColorPicker's input could be modified independently from the other Inputs by viewing the DOM only. Therefore I think it's acceptable keeping ${componentStyles};. However, I would remove
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
from attrs. It has no impact on the output and creates confusion.
Finally, we agree that how .attrs inherits values is confusing. This is why some cases call for ugly logic to get the intended result.
This can be all simplified by explicitly setting data-garden-id in the element component rather than .attrs. I hope we can get to that point in the future. 🤞
There was a problem hiding this comment.
My instinct is to leave these (false) COMPONENT_ID "overrides" in place. Following Garden's original implementation and expectation, these IDs were meant to provide highly targeted component styling overrides without unanticipated component side effects. When we rectify the current implementation with element-specified data-garden-ids (most likely in a future major) the existing COMPONENT_ID will serve as a breadcrumb for the original intention.
There was a problem hiding this comment.
To be clear, the clean up work would only apply to styled components extending another styled component.
Sounds good to me. We can follow-up on this in the future. 👍
Description
Big find/replace follow-on from #1986