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

feat: Add SidebarItem, sideBarGroup and sideBar components #6065

Merged
merged 27 commits into from
Nov 15, 2023

Conversation

manikantamavireddy
Copy link
Contributor

@manikantamavireddy manikantamavireddy commented Oct 29, 2023

Description

  • created SidebarItem component to render item in a group using LocalizedLink component and SibarGroup component to render list of items
  • composed Sidebar component from SidebarGroup to render multiple side bar groups

Validation

image
image

Related Issues

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

@vercel
Copy link

vercel bot commented Oct 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 15, 2023 1:21am

@manikantamavireddy manikantamavireddy marked this pull request as ready for review October 29, 2023 13:12
@manikantamavireddy manikantamavireddy requested a review from a team as a code owner October 29, 2023 13:12
types/sidebar.ts Outdated Show resolved Hide resolved
canerakdas

This comment was marked as resolved.

manikantamavireddy and others added 6 commits October 30, 2023 06:20
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: Augustin Mauroy <augustin.mauroy@outlook.fr>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
@canerakdas

This comment was marked as outdated.

@manikantamavireddy
Copy link
Contributor Author

Hey @canerakdas I have updated my code as per your suggestions could you be more specific about what styling changes that are required

canerakdas

This comment was marked as resolved.

@ovflowd
Copy link
Member

ovflowd commented Nov 1, 2023

I re-added my outdated comments @manikantamavireddy.

While checking these files, when I look at the current implementation, we check the activity status through a hook, which is quite simple to use. Instead of sending props from top to bottom, I think we need to check this control similarly in the SidebarItem via hook. What are your views @bmuenzenmeyer @ovflowd

I'm neutral. The Hook today is something we want to get rid of, since we want to get rid of the useLocale hook. I'm inclined into having the "activeItem" as a prop, mostly to keep the component simple and delegate this action to outside of the component.

manikantamavireddy and others added 5 commits November 2, 2023 07:12
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Co-authored-by: canerakdas <canerakdas@gmail.com>
Signed-off-by: manikanta mavireddy <56877683+manikantamavireddy@users.noreply.github.com>
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Besides of my two minor comments, can you rebase your branch? And would you mind updating your Storybook example to have these items?

image

Just so we can see how it'd look closer to a real page?

@bmuenzenmeyer
Copy link
Collaborator

@manikantamavireddy there are some small lints to clean up and then i think this is ready

@bmuenzenmeyer
Copy link
Collaborator

nice work @manikantamavireddy thanks for your contribution!

Merged via the queue into nodejs:main with commit 1a195e4 Nov 15, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Sidebar Component family
6 participants