Skip to content
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

Core: Implement Story Actions API and story options menu #29532

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions code/core/src/components/components/tooltip/ListItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export interface RightProps {

const Right = styled.span<RightProps>({
display: 'flex',
marginLeft: 10,
ghengeveld marked this conversation as resolved.
Show resolved Hide resolved
'& svg': {
height: 12,
width: 12,
Expand Down Expand Up @@ -135,10 +136,7 @@ const Item = styled.div<ItemProps>(
padding: '7px 10px',
display: 'flex',
alignItems: 'center',

'& > * + *': {
paddingLeft: 10,
},
gap: 10,
}),
({ theme, href, onClick }) =>
(href || onClick) && {
Expand Down
53 changes: 53 additions & 0 deletions code/core/src/manager-api/modules/stories.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import type {
API_ActionsState,
API_ActionsUpdate,
API_ComposedRef,
API_DocsEntry,
API_FilterFunction,
Expand Down Expand Up @@ -73,6 +75,7 @@ export interface SubState extends API_LoadedRefData {
storyId: StoryId;
internal_index?: API_PreparedStoryIndex;
viewMode: API_ViewMode;
actions: API_ActionsState;
status: API_StatusState;
filters: Record<string, API_FilterFunction>;
}
Expand Down Expand Up @@ -268,6 +271,17 @@ export interface SubAPI {
* @returns {Promise<void>} A promise that resolves when the preview has been set as initialized.
*/
setPreviewInitialized: (ref?: ComposedRef) => Promise<void>;
/**
* Updates the status of a collection of stories.
*
* @param {string} addonId - The ID of the addon to update.
* @param {StatusUpdate} update - An object containing the updated status information.
* @returns {Promise<void>} A promise that resolves when the status has been updated.
*/
experimental_updateActions: (
addonId: string,
update: API_ActionsUpdate | ((state: API_ActionsState) => API_ActionsUpdate)
) => Promise<void>;
/**
* Updates the status of a collection of stories.
*
Expand Down Expand Up @@ -631,6 +645,44 @@ export const init: ModuleFn<SubAPI, SubState> = ({
},

/* EXPERIMENTAL APIs */
experimental_updateActions: async (id, input) => {
const { actions, internal_index: index } = store.getState();
const newActions = { ...actions };

const update = typeof input === 'function' ? input(actions) : input;

if (!id || Object.keys(update).length === 0) {
return;
}
Comment on lines +654 to +656
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: missing validation for update object structure - could allow invalid action definitions through


Object.entries(update).forEach(([storyId, value]) => {
if (!storyId || typeof value !== 'object') {
return;
}
newActions[storyId] = { ...(newActions[storyId] || {}) };
if (value === null) {
delete newActions[storyId][id];
} else {
newActions[storyId][id] = value;
}

if (Object.keys(newActions[storyId]).length === 0) {
delete newActions[storyId];
}
});

await store.setState({ actions: newActions }, { persistence: 'session' });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider adding error handling for setState failure


if (index) {
// We need to re-prepare the index
await api.setIndex(index);

const refs = await fullAPI.getRefs();
Object.entries(refs).forEach(([refId, { internal_index, ...ref }]) => {
fullAPI.setRef(refId, { ...ref, storyIndex: internal_index }, true);
});
}
},
experimental_updateStatus: async (id, input) => {
const { status, internal_index: index } = store.getState();
const newStatus = { ...status };
Expand Down Expand Up @@ -903,6 +955,7 @@ export const init: ModuleFn<SubAPI, SubState> = ({
viewMode: initialViewMode,
hasCalledSetOptions: false,
previewInitialized: false,
actions: {},
status: {},
filters: config?.sidebar?.filters || {},
},
Expand Down
15 changes: 14 additions & 1 deletion code/core/src/manager/components/sidebar/Refs.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import React from 'react';

import { fn } from '@storybook/test';

import { ManagerContext } from '@storybook/core/manager-api';

import { standardData as standardHeaderData } from './Heading.stories';
Expand All @@ -8,6 +10,17 @@ import { Ref } from './Refs';
import { mockDataset } from './mockdata';
import type { RefType } from './types';

const managerContext: any = {
api: {
emit: fn().mockName('api::emit'),
on: fn().mockName('api::on'),
off: fn().mockName('api::off'),
},
state: {
docsOptions: {},
},
};

export default {
component: Ref,
title: 'Sidebar/Refs',
Expand All @@ -16,7 +29,7 @@ export default {
globals: { sb_theme: 'side-by-side' },
decorators: [
(storyFn: any) => (
<ManagerContext.Provider value={{ state: { docsOptions: {} } } as any}>
<ManagerContext.Provider value={managerContext}>
<IconSymbols />
{storyFn()}
</ManagerContext.Provider>
Expand Down
15 changes: 11 additions & 4 deletions code/core/src/manager/components/sidebar/Sidebar.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ const meta = {
storyId,
refId: DEFAULT_REF_ID,
refs: {},
actions: {},
status: {},
showCreateStoryButton: true,
isDevelopment: true,
Expand Down Expand Up @@ -236,8 +237,11 @@ export const StatusesCollapsed: Story = {
return {
...acc,
[id]: {
addonA: { status: 'warn', title: 'Addon A', description: 'We just wanted you to know' },
addonB: { status: 'error', title: 'Addon B', description: 'This is a big deal!' },
a: { status: 'warn', title: 'Visual changes', description: 'Look at that' },
b: { status: 'error', title: 'Component tests', description: 'This is a big deal!' },
c: { status: 'error', title: 'Accessibility violations', description: '', count: 2 },
d: { status: 'warn', title: 'Accessibility warnings', description: '', count: 1 },
e: { status: 'warn', title: 'Coverage', description: '', data: { score: '50%' } },
},
};
}
Expand All @@ -258,8 +262,11 @@ export const StatusesOpen: Story = {
return {
...acc,
[id]: {
addonA: { status: 'warn', title: 'Addon A', description: 'We just wanted you to know' },
addonB: { status: 'error', title: 'Addon B', description: 'This is a big deal!' },
a: { status: 'warn', title: 'Visual changes', description: 'Look at that' },
b: { status: 'error', title: 'Component tests', description: 'This is a big deal!' },
c: { status: 'error', title: 'Accessibility violations', description: '', count: 2 },
d: { status: 'warn', title: 'Accessibility warnings', description: '', count: 1 },
e: { status: 'warn', title: 'Coverage', description: '', data: { score: '50%' } },
},
};
}, {}),
Expand Down
30 changes: 20 additions & 10 deletions code/core/src/manager/components/sidebar/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { Search } from './Search';
import { SearchResults } from './SearchResults';
import { SidebarBottom } from './SidebarBottom';
import { TagsFilter } from './TagsFilter';
import { TEST_PROVIDER_ID } from './Tree';
import type { CombinedDataset, Selection } from './types';
import { useLastViewed } from './useLastViewed';

Expand Down Expand Up @@ -83,27 +82,36 @@ const Swap = React.memo(function Swap({
);
});

const useCombination = (
index: SidebarProps['index'],
indexError: SidebarProps['indexError'],
previewInitialized: SidebarProps['previewInitialized'],
status: SidebarProps['status'],
refs: SidebarProps['refs']
): CombinedDataset => {
const useCombination = ({
index,
indexError,
previewInitialized,
actions,
status,
refs,
}: {
index: SidebarProps['index'];
indexError: SidebarProps['indexError'];
previewInitialized: SidebarProps['previewInitialized'];
actions: SidebarProps['actions'];
status: SidebarProps['status'];
refs: SidebarProps['refs'];
}): CombinedDataset => {
const hash = useMemo(
() => ({
[DEFAULT_REF_ID]: {
index,
indexError,
previewInitialized,
actions,
status,
title: null,
id: DEFAULT_REF_ID,
url: 'iframe.html',
},
...refs,
}),
[refs, index, indexError, previewInitialized, status]
[refs, index, indexError, previewInitialized, actions, status]
);
// @ts-expect-error (non strict)
return useMemo(() => ({ hash, entries: Object.entries(hash) }), [hash]);
Comment on lines 116 to 117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This ts-expect-error should be fixed rather than suppressed. The CombinedDataset type likely needs updating to match the new structure.

Expand All @@ -113,6 +121,7 @@ const isRendererReact = global.STORYBOOK_RENDERER === 'react';

export interface SidebarProps extends API_LoadedRefData {
refs: State['refs'];
actions: State['actions'];
status: State['status'];
menu: any[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: menu is typed as any[]. Consider creating a proper type definition for menu items.

extra: Addon_SidebarTopType[];
Expand All @@ -132,6 +141,7 @@ export const Sidebar = React.memo(function Sidebar({
index,
indexJson,
indexError,
actions,
status,
previewInitialized,
menu,
Expand All @@ -146,7 +156,7 @@ export const Sidebar = React.memo(function Sidebar({
const [isFileSearchModalOpen, setIsFileSearchModalOpen] = useState(false);
// @ts-expect-error (non strict)
const selected: Selection = useMemo(() => storyId && { storyId, refId }, [storyId, refId]);
Comment on lines 157 to 158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Another ts-expect-error that should be addressed. The Selection type may need updating to handle null values properly.

const dataset = useCombination(index, indexError, previewInitialized, status, refs);
const dataset = useCombination({ index, indexError, previewInitialized, actions, status, refs });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: useCombination is called on every render since the object is created inline. Consider memoizing the options object.

const isLoading = !index && !indexError;
const lastViewedProps = useLastViewed(selected);
const { isMobile } = useLayout();
Expand Down
2 changes: 1 addition & 1 deletion code/core/src/manager/components/sidebar/StatusContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const useStatusSummary = (item: Item) => {
) {
for (const storyId of getDescendantIds(data, item.id, false)) {
for (const value of Object.values(status[storyId] || {})) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: check that value.status exists and is a valid API_StatusValue before using

summary.counts[value.status]++;
summary.counts[value.status] += value.count ?? 1;
summary.statuses[value.status][storyId] = summary.statuses[value.status][storyId] || [];
summary.statuses[value.status][storyId].push(value);
}
Expand Down
69 changes: 69 additions & 0 deletions code/core/src/manager/components/sidebar/StoryMenu.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import React from 'react';

import type { Meta, StoryObj } from '@storybook/react';
import { fn, userEvent } from '@storybook/test';

import { ManagerContext } from '@storybook/core/manager-api';

import { StoryMenu } from './StoryMenu';

const managerContext: any = {
api: {
emit: fn().mockName('api::emit'),
on: fn().mockName('api::on'),
off: fn().mockName('api::off'),
},
};
Comment on lines +10 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: managerContext is typed as 'any' which bypasses type checking. Consider creating proper types for the API interface.


const meta = {
component: StoryMenu,
args: {
storyId: 'story-id',
isSelected: false,
onSelectStoryId: () => {},
actions: {
addonA: {
title: 'Run component tests',
description: 'Run component tests for this story',
event: 'COMPONENT_TESTS_RUN',
},
addonB: {
title: 'Run accessibility tests',
event: 'A11Y_TESTS_RUN',
},
},
status: {
addonA: {
title: 'Accessibility violations',
status: 'error',
count: 3,
},
addonB: {
title: 'Visual changes',
status: 'warn',
},
},
statusIcon: null,
statusValue: 'unknown',
},
parameters: {
layout: 'fullscreen',
},
decorators: [
(Story) => <ManagerContext.Provider value={managerContext}>{Story()}</ManagerContext.Provider>,
(Story) => (
<div style={{ '--story-menu-visibility': 'visible', height: 220, width: 250 } as any}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Using CSS custom property with 'as any' type assertion. Consider defining proper types for the style object.

{Story()}
</div>
),
],
} satisfies Meta<typeof StoryMenu>;

export default meta;

type Story = StoryObj<typeof meta>;

export const Default: Story = {
play: async ({ canvas }) =>
userEvent.click(await canvas.findByRole('button', { name: 'Story menu' })),
};
Loading
Loading