feat(S2): show a console warning when content components are used standalone#9604
feat(S2): show a console warning when content components are used standalone#9604reidbarber wants to merge 4 commits intomainfrom
Conversation
| </svg> | ||
| </CenterBaseline> | ||
| <Text>{children}</Text> | ||
| <span>{children}</span> |
There was a problem hiding this comment.
I guess we probably added this for Skeleton loading capabilities. I can see if we go back to Text and update our hook to detect this instead.
There was a problem hiding this comment.
Yeah. I guess we're currently relying on the font styles being inherited from the div. You could potentially move those so they are directly applied to the Text instead?
|
Build successful! 🎉 |
| <PickerItem textValue="Emily" id="Emily"><Text>Emily</Text></PickerItem> | ||
| <PickerItem textValue="Amelia" id="Amelia"><Text>Amelia</Text></PickerItem> | ||
| <PickerItem textValue="Isla" id="Isla"><Text>Isla</Text></PickerItem> | ||
| <PickerItem textValue="Eva" id="Eva">Eva</PickerItem> |
There was a problem hiding this comment.
don't need textValue either then?
| export const Heading = forwardRef(// Wrapper around RAC Heading to unmount when hidden. | ||
| function Heading(props: HeadingProps, ref: DOMRef<HTMLHeadingElement>) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, HeadingContext); | ||
| useWarnIfNoStyles('Heading', props); |
There was a problem hiding this comment.
this is just checking props, would it be better to check that there's a context object? otherwise you wont' get the warning sometimes
you could pass the context to retrieve instead of props
useWarnIfNoStyles('Heading', HeadingContext);
then in the hook, useContext(Context) and just check if that returns an object at all
There was a problem hiding this comment.
It's happening after the props are merged from context so should be ok right?
There was a problem hiding this comment.
I wonder if this would be better?
useWarnIfNoStyles('Heading', ...allContexts);We could include SkeletonContext and anything else that comes along.
| console.warn( | ||
| `${componentName} is being used outside of a component that provides automatic styling. ` + | ||
| 'Consider using a standard HTML element instead (e.g., <h1>, <div>, <p>, etc.), ' + | ||
| 'and use the \'styles\' prop from the style macro to provide custom styles: https://react-spectrum.adobe.com/styling' |
There was a problem hiding this comment.
It would be className not styles if we are recommending using standard HTML elements. But seems like it could also be an OR instead of an AND? Like, they could use <Heading> or <Text> as long as they also provide styles. Which do we want to recommend? As you noticed, Text does have the benefit of working with skeleton as well so there could be cases where people want to use that.
| export const Heading = forwardRef(// Wrapper around RAC Heading to unmount when hidden. | ||
| function Heading(props: HeadingProps, ref: DOMRef<HTMLHeadingElement>) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, HeadingContext); | ||
| useWarnIfNoStyles('Heading', props); |
There was a problem hiding this comment.
It's happening after the props are merged from context so should be ok right?
| </svg> | ||
| </CenterBaseline> | ||
| <Text>{children}</Text> | ||
| <span>{children}</span> |
There was a problem hiding this comment.
Yeah. I guess we're currently relying on the font styles being inherited from the div. You could potentially move those so they are directly applied to the Text instead?
4 participants
When content components (
Text,Heading, etc.) are used standalone, they don't inherit any styles. This PR's change will show a warning in this case, so the user knows to use native HTML elements with the style macro instead.This change is also oriented to AI tools that frequently make this mistake, although it seems like they more often look at lint errors instead of browser errors. Not sure this is something we can catch with a lint plugin.
✅ Pull Request Checklist:
📝 Test Instructions:
See unit tests.
🧢 Your Project: