-
-
Notifications
You must be signed in to change notification settings - Fork 417
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: added company name after logo image and hover effect in nav-links #767
Conversation
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.
Hello, @Iamdivyak, thank you for opening a pull request.
Soon the maintainers/owner will review it and provide you with feedback/suggestions.
If you think it's something urgent, feel free to reach out to Tamal on Twitter.
Give us a β to show some support
Happy OpenSource π
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.
Also for the parent anchor tag that includes the Image and span - can we perhaps use Link
there ? We might also wanna add a flex based display because the image and the brand-name looks not aligned.
display: flex;
align-items: center;
This can be added to the parent div in order to keep the image and brand-name in sync.
made required changes |
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.
The reason we are changing the class names is to make sure we stay consistent across the styling and naming part !
The rest of the changes look fantastic.
src/components/Navbar.jsx
Outdated
<img | ||
src={ | ||
solidarity || | ||
"https://www.shareicon.net/data/512x512/2016/09/15/829452_user_512x512.png" | ||
} | ||
alt="Milan-logo" | ||
className="nav_bramhin_img" | ||
/> | ||
/> <span className="brand-name">MILAN</span> |
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.
And for this one, the classname can be nav_brand_name
done |
This pull request has been deployed to Vercel.
|
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.
Amazing work,
This looks good to me, will be merged soon π.
feat: added company name after the logo and hover effect in nav-links
Related Issue
Closes: #744
Changes made π·π»ββοΈ
Screenshots πΈ