-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(S2): show a console warning when content components are used standalone #9604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |
| */ | ||
|
|
||
| import {ContextValue, Keyboard as KeyboardAria, Header as RACHeader, Heading as RACHeading, TextContext as RACTextContext, SlotProps, Text as TextAria} from 'react-aria-components'; | ||
| import {createContext, forwardRef, ReactNode, useContext} from 'react'; | ||
| import {createContext, forwardRef, ReactNode, useContext, useEffect, useRef} from 'react'; | ||
| import {DOMRef, DOMRefValue} from '@react-types/shared'; | ||
| import {inertValue} from '@react-aria/utils'; | ||
| import {StyleString} from '../style/types'; | ||
|
|
@@ -37,11 +37,26 @@ interface HeadingProps extends Omit<ContentProps, 'children'> { | |
| level?: number | ||
| } | ||
|
|
||
| function useWarnIfNoStyles(componentName: string, props: {styles?: string, UNSAFE_className?: string, isHidden?: boolean}) { | ||
| let hasWarned = useRef(false); | ||
| useEffect(() => { | ||
| if (process.env.NODE_ENV !== 'production' && !hasWarned.current && !props.isHidden && !props.styles && !props.UNSAFE_className) { | ||
| 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' | ||
| ); | ||
| hasWarned.current = true; | ||
| } | ||
| }, [componentName, props.styles, props.UNSAFE_className, props.isHidden]); | ||
| } | ||
|
|
||
| export const HeadingContext = createContext<ContextValue<Partial<HeadingProps>, DOMRefValue<HTMLHeadingElement>>>(null); | ||
|
|
||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 then in the hook,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's happening after the props are merged from context so should be ok right?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this would be better? useWarnIfNoStyles('Heading', ...allContexts);We could include SkeletonContext and anything else that comes along. |
||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, ...otherProps} = props; | ||
| if (isHidden) { | ||
|
|
@@ -62,6 +77,7 @@ export const HeaderContext = createContext<ContextValue<Partial<ContentProps>, D | |
|
|
||
| export const Header = forwardRef(function Header(props: ContentProps, ref: DOMRef) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, HeaderContext); | ||
| useWarnIfNoStyles('Header', props); | ||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, ...otherProps} = props; | ||
| if (isHidden) { | ||
|
|
@@ -82,6 +98,7 @@ export const ContentContext = createContext<ContextValue<Partial<ContentProps>, | |
|
|
||
| export const Content = forwardRef(function Content(props: ContentProps, ref: DOMRef<HTMLDivElement>) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, ContentContext); | ||
| useWarnIfNoStyles('Content', props); | ||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, ...otherProps} = props; | ||
| if (isHidden) { | ||
|
|
@@ -101,6 +118,7 @@ export const TextContext = createContext<ContextValue<Partial<ContentProps>, DOM | |
|
|
||
| export const Text = forwardRef(function Text(props: ContentProps, ref: DOMRef) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, TextContext); | ||
| useWarnIfNoStyles('Text', props); | ||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, children, ...otherProps} = props; | ||
| let racContext = useContext(RACTextContext); | ||
|
|
@@ -135,6 +153,7 @@ export const KeyboardContext = createContext<ContextValue<Partial<ContentProps>, | |
|
|
||
| export const Keyboard = forwardRef(function Keyboard(props: ContentProps, ref: DOMRef) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, KeyboardContext); | ||
| useWarnIfNoStyles('Keyboard', props); | ||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, ...otherProps} = props; | ||
| if (isHidden) { | ||
|
|
@@ -154,6 +173,7 @@ export const FooterContext = createContext<ContextValue<Partial<ContentProps>, D | |
|
|
||
| export const Footer = forwardRef(function Footer(props: ContentProps, ref: DOMRef) { | ||
| [props, ref] = useSpectrumContextProps(props, ref, FooterContext); | ||
| useWarnIfNoStyles('Footer', props); | ||
| let domRef = useDOMRef(ref); | ||
| let {UNSAFE_className = '', UNSAFE_style, styles = '', isHidden, slot, ...otherProps} = props; | ||
| if (isHidden) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,6 @@ import {controlFont, getAllowedOverrides, StyleProps} from './style-utils' with | |
| import {createContext, forwardRef, ReactNode} from 'react'; | ||
| import {filterDOMProps} from '@react-aria/utils'; | ||
| import {style} from '../style' with {type: 'macro'}; | ||
| import {Text} from './Content'; | ||
| import {useDOMRef} from '@react-spectrum/utils'; | ||
| import {useIsSkeleton} from './Skeleton'; | ||
| import {useSpectrumContextProps} from './useSpectrumContextProps'; | ||
|
|
@@ -134,7 +133,7 @@ export const StatusLight = /*#__PURE__*/ forwardRef(function StatusLight(props: | |
| <circle r="50%" cx="50%" cy="50%" /> | ||
| </svg> | ||
| </CenterBaseline> | ||
| <Text>{children}</Text> | ||
| <span>{children}</span> | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| </div> | ||
| ); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /* | ||
| * Copyright 2026 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| import {act, pointerMap, render} from '@react-spectrum/test-utils-internal'; | ||
| import {ActionButton, ActionMenu, Button, ButtonGroup, Checkbox, Content, Dialog, DialogTrigger, Footer, Header, Heading, Keyboard, MenuItem, Text} from '../src'; | ||
| import React from 'react'; | ||
| import SaveFloppy from '../s2wf-icons/S2_Icon_SaveFloppy_20_N.svg'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| const getStyleWarning = (componentName: string) => `${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'; | ||
|
|
||
| describe('Content components', () => { | ||
| let warn: jest.SpyInstance; | ||
| let user; | ||
|
|
||
| beforeAll(() => { | ||
| jest.useFakeTimers(); | ||
| user = userEvent.setup({delay: null, pointerMap}); | ||
| }); | ||
|
|
||
| beforeEach(() => { | ||
| warn = jest.spyOn(global.console, 'warn').mockImplementation(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| warn.mockRestore(); | ||
| jest.clearAllMocks(); | ||
| act(() => jest.runAllTimers()); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| it.each` | ||
| Name | Component | ||
| ${'Heading'} | ${Heading} | ||
| ${'Header'} | ${Header} | ||
| ${'Content'} | ${Content} | ||
| ${'Text'} | ${Text} | ||
| ${'Keyboard'} | ${Keyboard} | ||
| ${'Footer'} | ${Footer} | ||
| `('should warn when $Name is used standalone', ({Name, Component}) => { | ||
| render(<Component>Test {Name}</Component>); | ||
| expect(warn).toHaveBeenCalledWith(getStyleWarning(Name)); | ||
| }); | ||
|
|
||
| it('should not warn when content components are used correctly', async () => { | ||
| let {getByRole} = render( | ||
| <DialogTrigger> | ||
| <ActionButton>Open dialog</ActionButton> | ||
| <Dialog> | ||
| {({close}) => ( | ||
| <> | ||
| <Heading slot="title">Dialog title</Heading> | ||
| <Header>Header text</Header> | ||
| <Content> | ||
| <p>This is the main content of the dialog.</p> | ||
| <ActionMenu> | ||
| <MenuItem | ||
| textValue="Copy" | ||
| onAction={() => alert('copy')}> | ||
| <Text slot="label">Copy</Text> | ||
| <Text slot="description">Copy the selected text</Text> | ||
| <Keyboard>⌘C</Keyboard> | ||
| </MenuItem> | ||
| </ActionMenu> | ||
| </Content> | ||
| <Footer><Checkbox>Don't show this again</Checkbox></Footer> | ||
| <ButtonGroup> | ||
| <Button onPress={close} variant="secondary">Cancel</Button> | ||
| <Button onPress={close} variant="accent"> | ||
| <SaveFloppy /> | ||
| <Text>Save</Text> | ||
| </Button> | ||
| </ButtonGroup> | ||
| </> | ||
| )} | ||
| </Dialog> | ||
| </DialogTrigger> | ||
| ); | ||
|
|
||
| // Open the dialog to render all content components | ||
| let trigger = getByRole('button'); | ||
| await user.click(trigger); | ||
| act(() => {jest.runAllTimers();}); | ||
|
|
||
| // Should not show warnings | ||
| expect(warn).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,7 +27,6 @@ import { | |
| TableHeader, | ||
| TableView, | ||
| TableViewProps, | ||
| Text, | ||
| TextField | ||
| } from '../src'; | ||
| import Edit from '../s2wf-icons/S2_Icon_Edit_20_N.svg'; | ||
|
|
@@ -210,16 +209,16 @@ describe('TableView', () => { | |
| autoFocus | ||
| defaultValue={item[column.id!]} | ||
| name={column.id! as string}> | ||
| <PickerItem textValue="Eva" id="Eva"><Text>Eva</Text></PickerItem> | ||
| <PickerItem textValue="Steven" id="Steven"><Text>Steven</Text></PickerItem> | ||
| <PickerItem textValue="Michael" id="Michael"><Text>Michael</Text></PickerItem> | ||
| <PickerItem textValue="Sara" id="Sara"><Text>Sara</Text></PickerItem> | ||
| <PickerItem textValue="Karina" id="Karina"><Text>Karina</Text></PickerItem> | ||
| <PickerItem textValue="Otto" id="Otto"><Text>Otto</Text></PickerItem> | ||
| <PickerItem textValue="Matt" id="Matt"><Text>Matt</Text></PickerItem> | ||
| <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> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need textValue either then? |
||
| <PickerItem textValue="Steven" id="Steven">Steven</PickerItem> | ||
| <PickerItem textValue="Michael" id="Michael">Michael</PickerItem> | ||
| <PickerItem textValue="Sara" id="Sara">Sara</PickerItem> | ||
| <PickerItem textValue="Karina" id="Karina">Karina</PickerItem> | ||
| <PickerItem textValue="Otto" id="Otto">Otto</PickerItem> | ||
| <PickerItem textValue="Matt" id="Matt">Matt</PickerItem> | ||
| <PickerItem textValue="Emily" id="Emily">Emily</PickerItem> | ||
| <PickerItem textValue="Amelia" id="Amelia">Amelia</PickerItem> | ||
| <PickerItem textValue="Isla" id="Isla">Isla</PickerItem> | ||
| </Picker> | ||
| )}> | ||
| <div>{item[column.id]}<ActionButton slot="edit" aria-label="Edit farmer"><Edit /></ActionButton></div> | ||
|
|
@@ -582,16 +581,16 @@ describe('TableView', () => { | |
| autoFocus | ||
| defaultValue={item[column.id!]} | ||
| name={column.id! as string}> | ||
| <PickerItem textValue="Eva" id="Eva"><Text>Eva</Text></PickerItem> | ||
| <PickerItem textValue="Steven" id="Steven"><Text>Steven</Text></PickerItem> | ||
| <PickerItem textValue="Michael" id="Michael"><Text>Michael</Text></PickerItem> | ||
| <PickerItem textValue="Sara" id="Sara"><Text>Sara</Text></PickerItem> | ||
| <PickerItem textValue="Karina" id="Karina"><Text>Karina</Text></PickerItem> | ||
| <PickerItem textValue="Otto" id="Otto"><Text>Otto</Text></PickerItem> | ||
| <PickerItem textValue="Matt" id="Matt"><Text>Matt</Text></PickerItem> | ||
| <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> | ||
| <PickerItem textValue="Steven" id="Steven">Steven</PickerItem> | ||
| <PickerItem textValue="Michael" id="Michael">Michael</PickerItem> | ||
| <PickerItem textValue="Sara" id="Sara">Sara</PickerItem> | ||
| <PickerItem textValue="Karina" id="Karina">Karina</PickerItem> | ||
| <PickerItem textValue="Otto" id="Otto">Otto</PickerItem> | ||
| <PickerItem textValue="Matt" id="Matt">Matt</PickerItem> | ||
| <PickerItem textValue="Emily" id="Emily">Emily</PickerItem> | ||
| <PickerItem textValue="Amelia" id="Amelia">Amelia</PickerItem> | ||
| <PickerItem textValue="Isla" id="Isla">Isla</PickerItem> | ||
| </Picker> | ||
| )}> | ||
| <div>{item[column.id]}<ActionButton slot="edit" aria-label="Edit farmer"><Edit /></ActionButton></div> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be
classNamenotstylesif 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 providestyles. Which do we want to recommend? As you noticed,Textdoes have the benefit of working with skeleton as well so there could be cases where people want to use that.