-
-
Couldn't load subscription status.
- Fork 4.5k
feat(cmdk): Add new command palette component #101551
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: master
Are you sure you want to change the base?
Changes from all commits
4eb4431
ee30b83
49d9f1c
4fe937b
992be4e
28dffa3
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 |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| import {createContext, useCallback, useContext, useMemo, useState} from 'react'; | ||
|
|
||
| import type {CommandPaletteAction} from './types'; | ||
|
|
||
| type CommandPaletteProviderProps = {children: React.ReactNode}; | ||
|
|
||
| type CommandPaletteStore = { | ||
| actions: CommandPaletteAction[]; | ||
| }; | ||
|
|
||
| type CommandPaletteConfig = { | ||
| registerActions: (actions: CommandPaletteAction[]) => void; | ||
| unregisterActions: (keys: string[]) => void; | ||
| }; | ||
|
|
||
| const CommandPaletteConfigContext = createContext<CommandPaletteConfig | null>(null); | ||
| const CommandPaletteStoreContext = createContext<CommandPaletteStore | null>(null); | ||
|
|
||
| export function useCommandPaletteConfiguration(): CommandPaletteConfig { | ||
| const ctx = useContext(CommandPaletteConfigContext); | ||
| if (ctx === null) { | ||
| throw new Error('Must be wrapped in CommandPaletteProvider'); | ||
| } | ||
| return ctx; | ||
| } | ||
|
|
||
| export function useCommandPaletteStore(): CommandPaletteStore { | ||
| const ctx = useContext(CommandPaletteStoreContext); | ||
| if (ctx === null) { | ||
| throw new Error('Must be wrapped in CommandPaletteProvider'); | ||
| } | ||
| return ctx; | ||
| } | ||
|
|
||
| export function CommandPaletteProvider({children}: CommandPaletteProviderProps) { | ||
| const [actions, setActions] = useState<CommandPaletteAction[]>([]); | ||
|
|
||
| const registerActions = useCallback((newActions: CommandPaletteAction[]) => { | ||
| setActions(prev => { | ||
| const result = [...prev]; | ||
|
|
||
| for (const newAction of newActions) { | ||
| const existingIndex = result.findIndex(action => action.key === newAction.key); | ||
|
|
||
| if (existingIndex >= 0) { | ||
| result[existingIndex] = newAction; | ||
| } else { | ||
| result.push(newAction); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| }); | ||
| }, []); | ||
|
|
||
| const unregisterActions = useCallback((keys: string[]) => { | ||
| setActions(prev => { | ||
| return prev.filter(action => !keys.includes(action.key)); | ||
| }); | ||
| }, []); | ||
|
|
||
| const config = useMemo<CommandPaletteConfig>( | ||
| () => ({ | ||
| registerActions, | ||
| unregisterActions, | ||
| }), | ||
| [registerActions, unregisterActions] | ||
| ); | ||
|
|
||
| const store = useMemo<CommandPaletteStore>( | ||
| () => ({ | ||
| actions, | ||
| }), | ||
| [actions] | ||
| ); | ||
malwilley marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| return ( | ||
| <CommandPaletteConfigContext.Provider value={config}> | ||
| <CommandPaletteStoreContext.Provider value={store}> | ||
| {children} | ||
| </CommandPaletteStoreContext.Provider> | ||
| </CommandPaletteConfigContext.Provider> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||
| import type {ReactNode} from 'react'; | ||||||
| import type {LocationDescriptor} from 'history'; | ||||||
|
|
||||||
| export type CommandPaletteAction = { | ||||||
|
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 think there are a couple of things that are mixed here that make the definitions a bit unreadable. How about something like I would have honestly also preferred if we could delegate rendering to every place that registers the action |
||||||
| /** Unique identifier for this action */ | ||||||
| key: string; | ||||||
| /** Primary text shown to the user */ | ||||||
| label: string; | ||||||
| /** Icon to render for this action */ | ||||||
| actionIcon?: ReactNode; | ||||||
|
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 already an icon :)
Suggested change
|
||||||
| /** Nested actions to show when this action is selected */ | ||||||
| children?: CommandPaletteAction[]; | ||||||
|
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.
Suggested change
An action that can have sub-actions :)? 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. Ah yes this is a better name! |
||||||
| /** Additional context or description */ | ||||||
| details?: string; | ||||||
| /** Whether this action should keep the modal open after execution */ | ||||||
| keepOpen?: boolean; | ||||||
|
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. Instead of a separate keepOpen, what if onAction could return an optional bool to keep the modal open? 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. Ooh I do like that better |
||||||
| /** Optional keywords to improve searchability */ | ||||||
| keywords?: string[]; | ||||||
| /** | ||||||
| * Execute a callback when the action is selected. | ||||||
| * Use the `to` prop if you want to navigate to a route. | ||||||
| */ | ||||||
| onAction?: () => void; | ||||||
| /** Section to group the action in the palette */ | ||||||
| section?: string; | ||||||
|
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. Since this influences grouping, should it be named something like |
||||||
| /** Navigate to a route when selected */ | ||||||
| to?: LocationDescriptor; | ||||||
| }; | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; | ||
|
|
||
| import * as modalActions from 'sentry/actionCreators/modal'; | ||
| import {CommandPaletteProvider} from 'sentry/components/commandPalette/context'; | ||
| import type {CommandPaletteAction} from 'sentry/components/commandPalette/types'; | ||
| import {CommandPaletteContent} from 'sentry/components/commandPalette/ui/content'; | ||
| import {useCommandPaletteActions} from 'sentry/components/commandPalette/useCommandPaletteActions'; | ||
|
|
||
| function RegisterActions({actions}: {actions: CommandPaletteAction[]}) { | ||
| useCommandPaletteActions(actions); | ||
| return null; | ||
| } | ||
|
|
||
| function GlobalActionsComponent({ | ||
| actions, | ||
| children, | ||
| }: { | ||
| actions: CommandPaletteAction[]; | ||
| children?: React.ReactNode; | ||
| }) { | ||
| return ( | ||
| <CommandPaletteProvider> | ||
| <RegisterActions actions={actions} /> | ||
| <CommandPaletteContent /> | ||
| {children} | ||
| </CommandPaletteProvider> | ||
| ); | ||
| } | ||
|
|
||
| const onChild = jest.fn(); | ||
| const onKeepOpen = jest.fn(); | ||
|
|
||
| const globalActions: CommandPaletteAction[] = [ | ||
| { | ||
| key: 'go-to-route', | ||
| label: 'Go to route', | ||
| to: '/target/', | ||
| section: 'Navigation', | ||
| }, | ||
| {key: 'other', label: 'Other', to: '/other/'}, | ||
| { | ||
| key: 'parent-action', | ||
| label: 'Parent action', | ||
| section: 'Other', | ||
| children: [ | ||
| { | ||
| key: 'child-action', | ||
| label: 'Child action', | ||
| onAction: onChild, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| key: 'keep-open', | ||
| label: 'Keep open', | ||
| section: 'Other', | ||
| onAction: onKeepOpen, | ||
| keepOpen: true, | ||
| }, | ||
| ]; | ||
|
|
||
| describe('CommandPaletteContent', () => { | ||
| beforeEach(() => { | ||
| jest.resetAllMocks(); | ||
| }); | ||
|
|
||
| it('clicking a link item navigates and closes modal', async () => { | ||
| const closeSpy = jest.spyOn(modalActions, 'closeModal'); | ||
| const {router} = render(<GlobalActionsComponent actions={globalActions} />); | ||
| await userEvent.click(await screen.findByRole('option', {name: 'Go to route'})); | ||
|
|
||
| await waitFor(() => expect(router.location.pathname).toBe('/target/')); | ||
| expect(closeSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('ArrowDown to a link item then Enter navigates and closes modal', async () => { | ||
| const closeSpy = jest.spyOn(modalActions, 'closeModal'); | ||
| const {router} = render(<GlobalActionsComponent actions={globalActions} />); | ||
| await screen.findByRole('textbox', {name: 'Search commands'}); | ||
| // First item should already be highlighted, arrow down will go highlight "other" | ||
| await userEvent.keyboard('{ArrowDown}{Enter}'); | ||
|
|
||
| await waitFor(() => expect(router.location.pathname).toBe('/other/')); | ||
| expect(closeSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('clicking action with children shows sub-items, backspace returns', async () => { | ||
| const closeSpy = jest.spyOn(modalActions, 'closeModal'); | ||
| render(<GlobalActionsComponent actions={globalActions} />); | ||
|
|
||
| // Open children | ||
| await userEvent.click(await screen.findByRole('option', {name: 'Parent action'})); | ||
|
|
||
| // Textbox changes placeholder to parent action label | ||
| await waitFor(() => { | ||
| expect(screen.getByRole('textbox', {name: 'Search commands'})).toHaveAttribute( | ||
| 'placeholder', | ||
| 'Parent action' | ||
| ); | ||
| }); | ||
|
|
||
| // Child actions are visible, global actions are not | ||
| expect(screen.getByRole('option', {name: 'Child action'})).toBeInTheDocument(); | ||
| expect(screen.queryByRole('option', {name: 'Parent action'})).not.toBeInTheDocument(); | ||
| expect(screen.queryByRole('option', {name: 'Go to route'})).not.toBeInTheDocument(); | ||
|
|
||
| // Hit Backspace on the input to go back | ||
| await userEvent.keyboard('{Backspace}'); | ||
|
|
||
| // Back to main actions | ||
| expect( | ||
| await screen.findByRole('option', {name: 'Parent action'}) | ||
| ).toBeInTheDocument(); | ||
| expect(screen.queryByRole('option', {name: 'Child action'})).not.toBeInTheDocument(); | ||
|
|
||
| expect(closeSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('clicking child sub-item runs onAction and closes modal', async () => { | ||
| const closeSpy = jest.spyOn(modalActions, 'closeModal'); | ||
| render(<GlobalActionsComponent actions={globalActions} />); | ||
| await userEvent.click(await screen.findByRole('option', {name: 'Parent action'})); | ||
| await userEvent.click(await screen.findByRole('option', {name: 'Child action'})); | ||
|
|
||
| expect(onChild).toHaveBeenCalled(); | ||
| expect(closeSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('keeps modal open after action with keepOpen flag', async () => { | ||
| const closeSpy = jest.spyOn(modalActions, 'closeModal'); | ||
| render(<GlobalActionsComponent actions={globalActions} />); | ||
| await userEvent.click(await screen.findByRole('option', {name: 'Keep open'})); | ||
| expect(closeSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
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.
The action and usage pattern here would be more suitable for a reducer than useState. That way all that you need to pass to the reducers is just the dispatch function and the state, and you won't need to deal with any memo problems