-
-
Notifications
You must be signed in to change notification settings - Fork 560
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 icons component and improve codebase structure #2318
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.
Thank you, Anmol-Baranwal, for creating this pull request and contributing to LinksHub! 💗
The maintainers will review this Pull Request and provide feedback as soon as possible! 😇
We appreciate your patience and contribution, Keep up the great work! 😀
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.
Excellent PR, @Anmol-Baranwal 💪 🚀 💯
Category content is not taking up full space of the sidebar. Could you please help to check that?
@aftabrehan Please check if the same issue is with the original URL, so we can solve it separately. If you could provide the screen size and instructions on how to check it, I'd be able to solve it properly. |
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.
LGTM
I'm using large screen and I can see that on 100%. Let me check the code if I can help. |
@Anmol-Baranwal, the issue (sidebar category height) is fixed in this commit. |
@rupali-codes @CBID2 @aftabrehan |
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.
LGTM
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.
@rupali-codes Should it appear on all routes, including subcategories, or only on the landing page? Let me know, and I will change it accordingly. |
I think it should be sticky, |
making it sticky and responsive
@rupali-codes I have kept it hidden for xs & sm. And made it slightly responsive for md.
|
to fix lint warnings
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.
But removing the text might confuse users because it provides context. If you still want it removed, please let me know, and I'll take care of it. Also, could you clarify what you mean by 'sticky'? Currently, it's floating (sticky) irrespective of scroll position, but are you suggesting it should be attached to the bottom of the page, visible to users as they scroll down? |
@rupali-codes |
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.
@Anmol-Baranwal it looks perfect, just one thing, there's a transition delay when changing the theme. And can yo try making the color voilet? ig it would look more attractive then
@rupali-codes @aftabrehan @CBID2 |
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.
Looks great
@rupali-codes |
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.
@Anmol-Baranwal there's a small bug, when you click on theme toggler on the resource page, and then click back on the toggler, its closing the sidebar menu
LinksHub.-.Brave.2024-04-20.12-33-02.mp4
Is this related to this PR? |
it's not on the main side, it belongs to this PR only |
@rupali-codes See the video here. |
Okay then, i'm merging this PR for now, |
Thank you for contributing to LinksHub! Please take a moment to rate this repo's DX on EddieHub's RepoRater and give it a star ⭐ |
Fixes Issue
fixes #2317, fixes #2315
Changes proposed
components/icons.tsx
Now:
fa
, andfa6
.classNames
to style the icons.assets/icons
other thanlogo.svg
priority
for images so that they're loaded faster.Note to reviewers
You can see that I've kept
assets/logo.svg
-> I've included it in the icons component but the viewport of the logo seems to be messy which is causing the issue. So, I used the previous one.