-
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 |
|---|---|---|
|
|
@@ -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. |
||
| </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.
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