-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat(SidebarItem): hover effect #7934
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: main
Are you sure you want to change the base?
feat(SidebarItem): hover effect #7934
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Pull Request Overview
This PR adds interactive hover effects to sidebar links, ensuring distinct visual feedback in both light and dark modes.
- Introduces hover background, text, and shadow styles for non-active sidebar items
- Adds smooth transitions and rounded corners to the sidebar item container and its nested elements
- Updates label, icon, and progressionIcon classes with consistent transition settings
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
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.
Why can't we do something like
&:hover {
&:not(.progression) .label {
@apply rounded-sm
bg-whatever-600
text-white;
}
}
Hi @avivkeller, Thank you for the feedback! I understand you're suggesting to restructure the hover effects like this:
I’ve implemented your suggested changes as follows:
on the place of current PR code :
Updated Hover Style:
Please let me know if this matches with your suggested intention, or if there are any further tweaks or improvements you’d like me to make! Also, should I update the existing one , if these chages are Okay ? Thanks again for the guidance and review! |
I think you should use the grey color you were using again |
This comment has been minimized.
This comment has been minimized.
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
packages/ui-components/Containers/Sidebar/SidebarItem/index.module.css
Outdated
Show resolved
Hide resolved
Hi @avivkeller & @AugustinMauroy, I’ve updated the sidebar hover to use the greyish background, matching the nav hover style as suggested. After Chages:I'd pushed all the chages. Let me know if there’s anything else you’d like me to adjust! |
I’ve reverted all unnecessary edits and kept only:
All other unintended changes have been removed. |
Hovering on the Learn pages feels a bit odd now in some circumstances: Looks like an unrelated bug? The hexagons seem to be weird on last and first entries apparently. We could address that on separate PRs I've also noticed, these menus on the blog page don't have an hover effect, would you mind, @Mohit5Upadhyay to make another contribution on a separate PR with that? Thanks! Also OOC, does it make sense to not include the arrow as part of the hover? (cc @avivkeller, maybe not, but just wondering...) |
+1 on including the arrow, my guess would be to wrap them in a div |
text-sm | ||
transition-colors | ||
duration-200 | ||
ease-in-out; |
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.
Unrelated
margin-bottom: 1px; | ||
|
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.
Unrelated
Thanks! @ovflowd |
…nded corners, and clean unrelated styles as per review
hi @avivkeller & @ovflowd , Regarding Arrow icon included in hover:
Also as per the suggestion I also removed unrelated chages but it causes: I pushed my changes for review , please suggest me more tweaks if they required to be! |
|
||
{/^https?:/.test(link) && <ArrowUpRightIcon className={styles.icon} />} | ||
<div className={`${styles.label} flex items-center gap-1.5`}> |
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.
Let's put these styles in the CSS file
@apply font-regular | ||
p-2 | ||
text-sm; | ||
p-2; |
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.
Can we revert this, unless it's important?
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7934 +/- ##
==========================================
- Coverage 75.42% 75.36% -0.06%
==========================================
Files 96 96
Lines 8354 8354
Branches 219 220 +1
==========================================
- Hits 6301 6296 -5
- Misses 2051 2056 +5
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Noted! Feel free to re-add those, as they are important, but please use tailwind to apply the margin |
Hi @avivkeller,
Let me know if there’s anything else you’d like me to refine! |
Description
This PR adds a hover effect to the links in the website’s left sidebar navigation. This PR resolves issue #7893 by introducing a hover effect for links both for
dark-mode
andlight-mode
Validation
sidebar-hover-effect-resolve.mp4
Related Issues
Fixes #7893
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.