-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Added a story for LeftNavBar component #14970
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
Added a story for LeftNavBar component #14970
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
TylerAPfledderer
left a comment
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.
Hey @alexandriaroberts!
Thank you for your contributions to the Ethereum website! 😄 I know this PR has been sitting for a while. The core team has many projects of focus and will reach this as soon as possible for a final review.
I took a look and offer suggested changes to ensure this new store file is consistent with the others in the project, as well as being practical in how it appropriately "re-enacts" this components current rendering in the project.
In addition to changes I provide within the meta object, I also think there should be only one story. The dropdown button is used more often than not right now, and increasing the max depth does not provide a major design change.
So in only having one story, we change the import alias from LeftNavbar to LeftNavBarComponent, so that the name of the single story is called LeftNavBar.
export const LeftNavBar: Story = {
args: {
tocItems: mockTocItems,
dropdownLinks: mockDropdownLinks,
maxDepth: 2,
},
}Also, condensing to a single story here minimizes the number of files to rebuild when needed in Chromatic for billing purposes.
| const meta: Meta<typeof LeftNavBar> = { | ||
| title: "Molecules / Navigation / LeftNavBar", | ||
| component: LeftNavBar, | ||
| parameters: { | ||
| layout: "centered", | ||
| }, | ||
| } |
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.
For consistency, we use the satisfies keyword as the most up-to-date approach in typing the meta object to ensure that required args aren't missing (not just invalid).
We also should better emulated how we expect this component to be rendered in context. We should allow the story layout to be fullscreen, and create a decorator around the rendering with a max width that matches the max width LeftNavBar is given on a page. Here, that max width is 400px. (See Roadmap page for example)
| const meta: Meta<typeof LeftNavBar> = { | |
| title: "Molecules / Navigation / LeftNavBar", | |
| component: LeftNavBar, | |
| parameters: { | |
| layout: "centered", | |
| }, | |
| } | |
| const meta = { | |
| title: "Molecules / Navigation / LeftNavBar", | |
| component: LeftNavBar, | |
| parameters: { | |
| layout: "fullscreen", | |
| }, | |
| decorators: [ | |
| (Story) => ( | |
| <div className="max-w-[496px]"> | |
| <Story /> | |
| </div> | |
| ), | |
| ], | |
| } satisfies Meta<typeof LeftNavBar> |
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.
Hi @TylerAPfledderer no worries really. I know they been busy, and thank you for reviewing it. It makes sense what you say so I will update that. :)
|
@pettinarip this is exposing an oddity with the And pushing any change here or updating this branch with |
…h 'add-story-for-LeftNavBar-component' of github.com:alexandriaroberts/ethereum-org-website into add-story-for-LeftNavBar-component Merge remote changes with local LeftNavBar component story#
|
This issue is stale because it has been open 30 days with no activity. |
|
Hey @alexandriaroberts, appreciate the work here though this component has recently been deprecated in favor of a general purpose TableOfContents component in #11650... closing this one out |

Created story tests for the LeftNavBar component
Description
Create story file /src/components/LeftNavBar/LeftNavBar.stories.tsx
Writes story testing render of LeftNavBar component in context of use
Related Issue
closes #14966