-
-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Search: Add docs headings to search results #33593
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: next
Are you sure you want to change the base?
Changes from all commits
8923b48
ae45742
e947ad1
81d5bf3
75fbe96
77ef62c
4328421
18d6b64
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 |
|---|---|---|
|
|
@@ -143,7 +143,7 @@ export interface SubAPI { | |
| selectStory: ( | ||
| kindOrId?: string, | ||
| story?: StoryId, | ||
| obj?: { ref?: string; viewMode?: API_ViewMode } | ||
| obj?: { ref?: string; viewMode?: API_ViewMode; scrollTo?: string } | ||
| ) => void; | ||
| /** | ||
| * Returns the current story's data, including its ID, kind, name, and parameters. | ||
|
|
@@ -631,14 +631,14 @@ export const init: ModuleFn<SubAPI, SubState> = ({ | |
| navigateWithQueryParams('/'); | ||
| }, | ||
| selectStory: (titleOrId = undefined, name = undefined, options = {}) => { | ||
| const { ref } = options; | ||
| const { ref, scrollTo } = options; | ||
| const { storyId, index, filteredIndex, refs, settings } = store.getState(); | ||
|
|
||
| const gotoStory = (entry?: API_HashEntry) => { | ||
| if (entry?.type === 'docs' || entry?.type === 'story') { | ||
| store.setState({ settings: { ...settings, lastTrackedStoryId: entry.id } }); | ||
| navigateWithQueryParams( | ||
| `/${entry.type}/${entry.refId ? `${entry.refId}_${entry.id}` : entry.id}` | ||
| `/${entry.type}/${entry.refId ? `${entry.refId}_${entry.id}` : entry.id}${scrollTo ? `#${scrollTo}` : ''}` | ||
|
Contributor
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 didn't appear to work when I tested your prototype. I did not get scrolled to the anchor ID. Could you please add tests for to ensure this function works as intended? This will help know whether the logic here is faulty or whether the anchor ID computation is faulty. |
||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,8 +43,9 @@ const options = { | |
| maxPatternLength: 32, | ||
| minMatchCharLength: 1, | ||
| keys: [ | ||
| { name: 'name', weight: 0.7 }, | ||
| { name: 'name', weight: 0.6 }, | ||
| { name: 'path', weight: 0.3 }, | ||
| { name: 'headings', weight: 0.1 }, | ||
| ], | ||
| } as FuseOptions<SearchItem>; | ||
|
|
||
|
|
@@ -161,6 +162,7 @@ export type SearchProps = { | |
| dataset: CombinedDataset; | ||
| enableShortcuts?: boolean; | ||
| getLastViewed: () => Selection[]; | ||
| updateLastViewed?: (story: { storyId: string; refId: string; anchor?: string }) => void; | ||
| initialQuery?: string; | ||
| searchBarContent?: ReactNode; | ||
| searchFieldContent?: ReactNode; | ||
|
|
@@ -172,6 +174,7 @@ export const Search = React.memo<SearchProps>(function Search({ | |
| dataset, | ||
| enableShortcuts = true, | ||
| getLastViewed, | ||
| updateLastViewed, | ||
| initialQuery = '', | ||
| searchBarContent, | ||
| searchFieldContent, | ||
|
|
@@ -184,25 +187,52 @@ export const Search = React.memo<SearchProps>(function Search({ | |
| const searchShortcut = api ? shortcutToHumanString(api.getShortcutKeys().search) : '/'; | ||
|
|
||
| const makeFuse = useCallback(() => { | ||
| const list = dataset.entries.reduce<SearchItem[]>((acc, [refId, { index, allStatuses }]) => { | ||
| const list: SearchItem[] = []; | ||
|
|
||
| for (const [refId, { index, allStatuses }] of dataset.entries) { | ||
| if (!index) { | ||
| continue; | ||
| } | ||
|
|
||
| const groupStatus = getGroupStatus(index || {}, allStatuses ?? {}); | ||
| const datasetValues = Object.values(index); | ||
|
|
||
| if (index) { | ||
| acc.push( | ||
| ...Object.values(index).map((item) => { | ||
| const storyStatuses = allStatuses?.[item.id]; | ||
| const mostCriticalStatusValue = storyStatuses | ||
| ? getMostCriticalStatusValue(Object.values(storyStatuses).map((s) => s.value)) | ||
| : null; | ||
| return { | ||
| ...searchItem(item, dataset.hash[refId]), | ||
| status: mostCriticalStatusValue ?? groupStatus[item.id] ?? null, | ||
| }; | ||
| }) | ||
| ); | ||
| for (const datasetValue of datasetValues) { | ||
| const storyStatuses = allStatuses?.[datasetValue.id]; | ||
| const mostCriticalStatusValue = storyStatuses | ||
| ? getMostCriticalStatusValue(Object.values(storyStatuses).map((s) => s.value)) | ||
| : null; | ||
|
|
||
| list.push({ | ||
| ...searchItem(datasetValue, dataset.hash[refId]), | ||
| status: mostCriticalStatusValue ?? groupStatus[datasetValue.id] ?? null, | ||
| }); | ||
|
Comment on lines
+200
to
+209
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. Mainly did the first refactor commit because it was very frustrating integrating my code additions into this reducer. If the reviewer thinks this is unnecessary, I can try to revert it and apply my additions in-between
Contributor
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. nit: I'd prefer the reducer be kept if possible, for diff readability and preserving git history. Not critical though. |
||
|
|
||
| // Narrow down type to more specific API_DocsEntry with headings | ||
| if (datasetValue.type !== 'docs') { | ||
| continue; | ||
| } | ||
|
|
||
| if (!globalThis?.FEATURES?.experimentalSearchDocsHeadings) { | ||
| continue; | ||
| } | ||
|
|
||
| const headings = datasetValue.headings ?? []; | ||
| headings.forEach((heading: string) => { | ||
| const searchItemRef = searchItem(datasetValue, dataset.hash[refId]); | ||
| const namePostfix = searchItemRef.path?.[0] === heading ? '' : ` / ${heading}`; | ||
|
|
||
| list.push({ | ||
| ...searchItemRef, | ||
| // TODO add comment about why -> fuse breaks if id is not unique | ||
| id: `${datasetValue.id}#${heading.replaceAll(' ', '-').toLowerCase()}`, | ||
| name: `${datasetValue.name}${namePostfix}`, | ||
| status: mostCriticalStatusValue || groupStatus[datasetValue.id] || null, | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }); | ||
| }); | ||
| } | ||
| return acc; | ||
| }, []); | ||
| } | ||
|
|
||
| return new Fuse(list, options); | ||
| }, [dataset]); | ||
|
|
||
|
|
@@ -247,9 +277,17 @@ export const Search = React.memo<SearchProps>(function Search({ | |
| const onSelect = useCallback( | ||
| (selectedItem: DownshiftItem) => { | ||
| if (isSearchResult(selectedItem)) { | ||
| const { id, refId } = selectedItem.item; | ||
| // @ts-expect-error (non strict) | ||
| api?.selectStory(id, undefined, { ref: refId !== DEFAULT_REF_ID && refId }); | ||
| const { id: rawId, refId } = selectedItem.item; | ||
| const [storyId, anchor] = rawId.split('#'); | ||
|
|
||
| updateLastViewed?.({ storyId, refId, anchor }); | ||
|
Contributor
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. Why does that differ from before? What was the previous mechanism for updating 'last viewed'? Can it be made to always support additional metadata like header anchors? |
||
|
|
||
| api?.selectStory(storyId, undefined, { | ||
| // @ts-expect-error (non strict) | ||
| ref: refId !== DEFAULT_REF_ID && refId, | ||
| scrollTo: anchor, | ||
| }); | ||
|
|
||
| // @ts-expect-error (non strict) | ||
| inputRef.current.blur(); | ||
| showAllComponents(false); | ||
|
|
@@ -348,16 +386,33 @@ export const Search = React.memo<SearchProps>(function Search({ | |
| const lastViewed = !input && getLastViewed(); | ||
| if (lastViewed && lastViewed.length) { | ||
| // @ts-expect-error (non strict) | ||
| results = lastViewed.reduce((acc, { storyId, refId }) => { | ||
| results = lastViewed.reduce((acc, { storyId, refId, anchor }) => { | ||
| const data = dataset.hash[refId]; | ||
| if (data && data.index && data.index[storyId]) { | ||
| const story = data.index[storyId]; | ||
| const item = story.type === 'story' ? data.index[story.parent] : story; | ||
| const entryId = anchor ? `${item.id}#${anchor}` : item.id; | ||
| // prevent duplicates | ||
| // @ts-expect-error (non strict) | ||
| if (!acc.some((res) => res.item.refId === refId && res.item.id === item.id)) { | ||
| if (!acc.some((res) => res.item.refId === refId && res.item.id === entryId)) { | ||
| const baseItem = searchItem(item, dataset.hash[refId]); | ||
| let resultItem = baseItem; | ||
| if (anchor && item.type === 'docs') { | ||
| const matchingHeading = item.headings?.find( | ||
| (h: string) => h.replaceAll(' ', '-').toLowerCase() === anchor | ||
|
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. probably a better way to do it, CodeRabbit has the same feeling 😄 Is there some kind of "getStorySlug()" or something?
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. maybe related to #34271 and I need to wait?
Contributor
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. #34271 is now merged, good news :) I feel there should be a way to compute a unique ID for a story, yes. If there is and it's not reachable for your code, we have a
Contributor
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. https://deepwiki.com/ can also help do deep searches for SB codebase utilities, in natural language, for free. |
||
| ); | ||
| if (matchingHeading) { | ||
| const namePostfix = | ||
| baseItem.path?.[0] === matchingHeading ? '' : ` / ${matchingHeading}`; | ||
| resultItem = { | ||
| ...baseItem, | ||
| id: entryId, | ||
| name: `${item.name}${namePostfix}`, | ||
| }; | ||
| } | ||
| } | ||
| // @ts-expect-error (non strict) | ||
| acc.push({ item: searchItem(item, dataset.hash[refId]), matches: [], score: 0 }); | ||
| acc.push({ item: resultItem, matches: [], score: 0 }); | ||
| } | ||
| } | ||
| return acc; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,36 +6,40 @@ import { type FunctionInterpolation, styled } from 'storybook/theming'; | |
| import { UseSymbol } from './IconSymbols.tsx'; | ||
| import { CollapseIcon } from './components/CollapseIcon.tsx'; | ||
|
|
||
| export const TypeIcon = styled.svg<{ type: 'component' | 'story' | 'test' | 'group' | 'document' }>( | ||
| ({ theme, type }) => ({ | ||
| width: 14, | ||
| height: 14, | ||
| flex: '0 0 auto', | ||
| color: (() => { | ||
| if (type === 'group') { | ||
| return theme.base === 'dark' ? theme.color.primary : theme.color.ultraviolet; | ||
| } | ||
|
|
||
| if (type === 'component') { | ||
| return theme.color.secondary; | ||
| } | ||
|
|
||
| if (type === 'document') { | ||
| return theme.base === 'dark' ? theme.color.gold : '#ff8300'; | ||
| } | ||
|
|
||
| if (type === 'story') { | ||
| return theme.color.seafoam; | ||
| } | ||
|
|
||
| if (type === 'test') { | ||
| return theme.color.green; | ||
| } | ||
|
|
||
| return 'currentColor'; | ||
| })(), | ||
| }) | ||
| ); | ||
| export const TypeIcon = styled.svg<{ | ||
| type: 'component' | 'story' | 'test' | 'group' | 'document' | 'heading'; | ||
| }>(({ theme, type }) => ({ | ||
| width: 14, | ||
| height: 14, | ||
| flex: '0 0 auto', | ||
| color: (() => { | ||
| if (type === 'group') { | ||
| return theme.base === 'dark' ? theme.color.primary : theme.color.ultraviolet; | ||
| } | ||
|
|
||
| if (type === 'component') { | ||
| return theme.color.secondary; | ||
| } | ||
|
|
||
| if (type === 'document') { | ||
| return theme.base === 'dark' ? theme.color.gold : '#ff8300'; | ||
| } | ||
|
|
||
| if (type === 'story') { | ||
| return theme.color.seafoam; | ||
| } | ||
|
|
||
| if (type === 'heading') { | ||
| return theme.color.secondary; | ||
| } | ||
|
Comment on lines
+32
to
+34
Contributor
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. At the moment I'm not seeing headings in the search results; only docs pages. But I prefer to see the entries referenced as docs pages, with a description under that highlights the matching content. That makes more sense to me anyway. |
||
|
|
||
| if (type === 'test') { | ||
| return theme.color.green; | ||
| } | ||
|
|
||
| return 'currentColor'; | ||
| })(), | ||
| })); | ||
|
|
||
| const commonNodeStyles: FunctionInterpolation<{ depth?: number; isExpandable?: boolean }> = ({ | ||
| theme, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ export function getPath(item: Item, ref: Pick<RefType, 'id' | 'title' | 'index'> | |
| } | ||
|
|
||
| export const searchItem = (item: Item, ref: Parameters<typeof getPath>[1]): SearchItem => { | ||
| return { ...item, refId: ref.id, path: getPath(item, ref) }; | ||
| return { ...item, refId: ref.id, name: item.name, path: getPath(item, ref) }; | ||
|
Contributor
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. nit: This change is likely not needed as the item spread would pass the name. Is |
||
| }; | ||
|
|
||
| export function cycle<T>(array: T[], index: number, delta: number): number { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| import { MANAGER_INERT_ATTRIBUTE_CHANGED, TELEMETRY_ERROR } from 'storybook/internal/core-events'; | ||
| import { | ||
| MANAGER_INERT_ATTRIBUTE_CHANGED, | ||
| NAVIGATE_URL, | ||
| TELEMETRY_ERROR, | ||
| } from 'storybook/internal/core-events'; | ||
|
|
||
| import { global } from '@storybook/global'; | ||
|
|
||
|
|
@@ -47,6 +51,10 @@ export function setup() { | |
| document.body.removeAttribute('inert'); | ||
| } | ||
| }); | ||
|
|
||
| channel.on(NAVIGATE_URL, (hash: string) => { | ||
| document.querySelector(hash)?.scrollIntoView({ behavior: 'smooth' }); | ||
| }); | ||
|
Comment on lines
+55
to
+57
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. Kinda feels like I'm misusing this.. the weird thing is that there is already behavior to scrollIntoView() a sub-heading when switching stories. if you search a sub heading on the same docs page you currently are, the scrollIntoView() logic does not get triggerd so I needed this code to fix it
Contributor
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. Agree, this feels odd. How does tocbot behave in that regard? What's the code path responsible for scrolling when clicking a tocbot heading in a docs page? |
||
| }); | ||
|
|
||
| // handle all uncaught StorybookError at the root of the application and log to telemetry if applicable | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ import * as R from 'react-router-dom'; | |
|
|
||
| import type { LinkProps, NavigateOptions, RenderData } from './types.ts'; | ||
| import { getMatch, parsePath, queryFromLocation } from './utils.ts'; | ||
| import { addons } from '../manager-api/index.ts'; | ||
| import { NAVIGATE_URL } from '../core-events/index.ts'; | ||
|
Comment on lines
+10
to
+11
Contributor
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. IIRC if you are importing across core modules, you should use |
||
|
|
||
| const { document } = global; | ||
|
|
||
|
|
@@ -56,6 +58,14 @@ export const useNavigate = () => { | |
| } | ||
| if (typeof to === 'string') { | ||
| const target = plain ? to : `?path=${to}`; | ||
| const [search, hash] = target.split('#'); | ||
|
|
||
| if (search === document.location.search && hash) { | ||
| addons.getChannel().emit(NAVIGATE_URL, `#${hash}`); | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
|
Comment on lines
+61
to
+68
Contributor
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. q: I'm not sure what this piece of code is responsible for. Could you please provide JS comments to explain its role? |
||
| return navigate(target, options); | ||
| } | ||
| if (typeof to === 'number') { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -532,6 +532,13 @@ export interface StorybookConfigRaw { | |
| */ | ||
| experimentalRSC?: boolean; | ||
|
|
||
| /** | ||
| * Adds docs story subheadings to the search index. | ||
| * | ||
| * @experimental This feature is in early development and may change significantly in future releases. | ||
| */ | ||
| experimentalSearchDocsHeadings?: boolean; | ||
|
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. since there are e2e tests, do we need to activate this by default in the react vite ts sandbox?
Contributor
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. Sure, if we add an experimental flag, then we can enable it in some sandboxes. |
||
|
|
||
| /** | ||
| * @temporary This feature flag is a migration assistant, and is scheduled to be removed. | ||
| * | ||
|
|
||
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.
I'm not sure adding headings to index entries is the right approach.
If we want these headings to be used exclusively for Search, I'd prefer us to have a separate search index (because that makes it easy to change search index internals, they are entirely private to Storybook; whereas the Story Index is a very public API).
Ultimately, if we wanted to expand search indexing to contain whole pages, then we would end up having to reproduce all docs content into the Story Index, which would be bad for current applications like serving
/index.json.