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

UI: Simple tag filtering #29333

Open
wants to merge 23 commits into
base: next
Choose a base branch
from
Open

UI: Simple tag filtering #29333

wants to merge 23 commits into from

Conversation

shilman
Copy link
Member

@shilman shilman commented Oct 11, 2024

Closes #

What I did

Design

image
Implementation
  • Shows number of selected tags
  • Shows the play-fn built in tag (if any stories have play functions) but hides other "system" tags
  • Scrolling bug when you remove all tags
image

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

  1. Open the UI Sandbox
  2. Try the various filters

Documentation

  • Needs documentation

🦋 Canary release

This pull request has been released as version 0.0.0-pr-29333-sha-c7603f3d. Try it out in a new sandbox by running npx storybook@0.0.0-pr-29333-sha-c7603f3d sandbox or in an existing project with npx storybook@0.0.0-pr-29333-sha-c7603f3d upgrade.

More information
Published version 0.0.0-pr-29333-sha-c7603f3d
Triggered by @shilman
Repository storybookjs/storybook
Branch shilman/simple-tag-filter
Commit c7603f3d
Datetime Mon Oct 14 06:50:15 UTC 2024 (1728888615)
Workflow run 11322402774

To request a new release of this pull request, mention the @storybookjs/core team.

core team members can create a new canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=29333

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 78.7 MB 78.7 MB 0 B 1.28 0%
initSize 147 MB 147 MB 3.69 kB -0.78 0%
diffSize 68.3 MB 68.3 MB 3.69 kB -0.79 0%
buildSize 6.79 MB 6.8 MB 3.58 kB 1.19 0.1%
buildSbAddonsSize 1.5 MB 1.5 MB 16 B -0.79 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 1.83 MB 1.84 MB 3.56 kB 43.1 0.2%
buildSbPreviewSize 270 kB 270 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 3.8 MB 3.8 MB 3.58 kB 27.35 0.1%
buildPreviewSize 2.99 MB 2.99 MB 0 B 0.8 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 6.2s 20.8s 14.6s 1.34 🔺70.2%
generateTime 21s 20.3s -710ms -0.24 -3.5%
initTime 14.9s 13.5s -1s -318ms -0.21 -9.7%
buildTime 8s 9.6s 1.6s 0.05 16.8%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 6.4s 6.9s 513ms 0.03 7.4%
devManagerResponsive 4.3s 4.4s 62ms -0.06 1.4%
devManagerHeaderVisible 747ms 604ms -143ms -0.32 -23.7%
devManagerIndexVisible 783ms 645ms -138ms -0.25 -21.4%
devStoryVisibleUncached 1s 1s 34ms -0.02 3.1%
devStoryVisible 793ms 645ms -148ms -0.26 -22.9%
devAutodocsVisible 623ms 515ms -108ms -0.33 -21%
devMDXVisible 592ms 539ms -53ms -0.14 -9.8%
buildManagerHeaderVisible 656ms 566ms -90ms -0.07 -15.9%
buildManagerIndexVisible 714ms 568ms -146ms -0.16 -25.7%
buildStoryVisible 711ms 598ms -113ms -0.34 -18.9%
buildAutodocsVisible 440ms 460ms 20ms -0.52 4.3%
buildMDXVisible 474ms 530ms 56ms 0.15 10.6%

Greptile Summary

This PR introduces a simple tag filtering feature to the Storybook UI, enhancing the sidebar's functionality for filtering stories based on tags.

  • Added new TagsFilter and TagsFilterPanel components in /code/core/src/manager/components/sidebar/
  • Updated Sidebar component to integrate tag filtering and remove 'Create New Story' button
  • Implemented logic to filter stories based on selected tags using experimental_setFilter in TagsFilter.tsx
  • Renamed STATIC_FILTER to TAG_FILTERS in common-manager.ts for consistency
  • Added new stories for TagsFilter and TagsFilterPanel components to showcase different states

Copy link

nx-cloud bot commented Oct 11, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 812f9d9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@shilman shilman changed the title UI WIP UI: Simple tag filtering Oct 12, 2024
@shilman shilman mentioned this pull request Oct 12, 2024
14 tasks
Copy link
Contributor

@Sidnioulz Sidnioulz left a comment

Choose a reason for hiding this comment

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

🤩 it's looking great!

I have a feeling that some users will want to filter by entry type (docs only or stories only). Do you reckon it could make sense, in a separate branch, to expose them as tags (type:story and type:docs) like you expose play-fn?

import { TagsFilter } from './TagsFilter';

const meta = {
component: TagsFilter,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you have to put the title explicitly to get these components to land in the same sidebar entry as the other Sidebar components?

image vs image

Copy link
Member Author

Choose a reason for hiding this comment

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

I want our Storybooks to use auto-title as much as possible. I'll move the other stories over in a separate PR.

code/core/src/manager/components/sidebar/Sidebar.tsx Outdated Show resolved Hide resolved
play: Open.play,
};

export const OpenEmpty: Story = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would if there shoould also be an empty state for when only internal tags are present, and users haven't yet defined custom tags? You mentioned wanting to show the play-fn tag but it would be a shame if it got in the way of feature onboarding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call

@shilman shilman marked this pull request as ready for review October 12, 2024 13:39
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

9 file(s) reviewed, 16 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 25 to 28
// we can filter out the primary story, but we still want to show autodocs
(tags.includes('dev') || item.type === 'docs') &&
tags.filter((tag) => excludeTags[tag]).length === 0
tags.filter((tag) => staticExcludeTags[tag]).length === 0
);
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 extracting this logic into a separate function for better readability

Comment on lines +135 to +136
const isDevelopment = global.CONFIG_TYPE === 'DEVELOPMENT';
const isRendererReact = global.STORYBOOK_RENDERER === 'react';
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 using environment variables or a configuration file for these global flags

@@ -126,13 +163,15 @@ export const Sidebar = React.memo(function Sidebar({
enableShortcuts = true,
refs = {},
onMenuClick,
showCreateStoryButton,
showCreateStoryButton = isDevelopment && isRendererReact,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Default value depends on global variables, which might cause issues if they're not set correctly

Comment on lines 123 to 133
const updateQueryParams = (params: Record<string, string>) => {
const url = new URL(window.location.href);
Object.entries(params).forEach(([key, value]) => {
if (value) {
url.searchParams.set(key, value);
} else {
url.searchParams.delete(key);
}
});
window.history.pushState({}, '', url);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

style: This function modifies the browser history directly. Consider using a routing library or API for better maintainability


const meta = {
component: TagsFilter,
tags: ['haha'],
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 removing this tag as it's not relevant to the component's functionality

<WithTooltip
placement="bottom"
trigger="click"
onVisibleChange={setExpanded}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The onVisibleChange prop is set to setExpanded, but expanded state is not used anywhere in the component. Consider removing this state if it's not needed.

Comment on lines +9 to +12
toggleTag: fn(),
api: {
getDocsUrl: () => 'https://storybook.js.org/docs/',
} 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: Consider using a more specific type for the api object instead of 'any'

toggleTag,
}: TagsFilterPanelProps) => {
const theme = useTheme();
const userTags = allTags.filter((tag) => tag === 'play-fn' || !BUILT_IN_TAGS.has(tag)).toSorted();
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 using Array.prototype.sort() instead of .toSorted() for better browser compatibility

return {
id,
title: tag,
right: <input type="checkbox" id={id} name={id} value={tag} checked={checked} />,
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 using a controlled component for the checkbox

Comment on lines +26 to +29
// FIXME: This is the actual `index.json` index where the `index` below
// is actually the stories hash. We should fix this up and make it consistent.
// eslint-disable-next-line @typescript-eslint/naming-convention
internal_index,
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 renaming 'internal_index' to a more descriptive name like 'indexJson' for clarity

@shilman shilman self-assigned this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants