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.

Sign up for GitHub

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

Jump to bottom

Refactor/sidebar #1441

Merged
merged 72 commits into from
Dec 10, 2024
Merged

Refactor/sidebar #1441

merged 72 commits into from
Dec 10, 2024

Conversation

rockingrohit9639
Copy link
Contributor

Issue - #1434

DonKoko
DonKoko requested changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@DonKoko DonKoko left a comment •
edited by rockingrohit9639
Loading

Choose a reason for hiding this comment

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

  • Sidebar items dont have the hover effect like in the demo with the backgournd changing
  • Sidebar items should have font-medium
  • User and Workspace dropdowns need to have a border around the trigger. Our standard border color and radius
  • the Dropdown menus themselves, currently have rounded-lg which is too much. Should be just rounded
  • Remove "home" icon which is next to the "fold sidebar" icon
  • opening workspace selector with a folded sidebar has a spacing bug
  • can you please set the color of the icons in the sidebar to text-gray-500. I want to see how they look
  • Asset labels icon is broken
  • The folding/opening should work like before with the userPrefs cookie and optimistic UI
  • I dont like how the padding is on the whole menu and not on the items. If you look at the demo the padding on the box is way less and the hover is rounded. Should be the same here. Same for the user dropdown
    image

app/components/icons/library.tsx Show resolved Hide resolved
@rockingrohit9639 rockingrohit9639 self-assigned this Dec 6, 2024
DonKoko
DonKoko requested changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@DonKoko DonKoko left a comment

Choose a reason for hiding this comment

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

Added a few comments. Also the issue we discussed in discord is pending.

app/components/layout/sidebar/notice-card.tsx Outdated Show resolved Hide resolved
app/components/layout/sidebar/parent-nav-item.tsx Show resolved Hide resolved
app/hooks/use-is-route-active.tsx Outdated Show resolved Hide resolved
app/hooks/use-mobile.ts Outdated Show resolved Hide resolved
@DonKoko DonKoko merged commit 564ee85 into Shelf-nu:main Dec 10, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DonKoko DonKoko DonKoko approved these changes

Assignees

@rockingrohit9639 rockingrohit9639

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rockingrohit9639 @carlosvirreira @DonKoko