-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Added hover animations for nav icons #11075
Conversation
… Downgraded the version back to 4.14.195 to maintain consistency.
✅ ethereum-org-website-dev deploy preview ready
|
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.
Hey @rohan9024! I have some suggestions below for now.
src/components/Nav/index.tsx
Outdated
<Box mr={2} mt={2}> | ||
{" "} | ||
{/* Add spacing between the text and the icon */} | ||
<Icon | ||
as={MdLanguage} | ||
style={{ | ||
transition: "transform 0.3s ease-in-out, color 0.2s", // Apply the transition to the icon | ||
transform: languagesHover | ||
? "rotate(30deg)" | ||
: "rotate(0deg)", // Rotate the icon to 30 degrees on hover | ||
color: languagesHover ? "blue" : "inherit", // Change color to blue on hover | ||
}} | ||
/> | ||
</Box> | ||
<span>Languages {i18n.language.toUpperCase()}</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.
This is excessive. The Chakra Button
which ButtonLink
uses has the props leftIcon
and rightIcon
which handle alignment of an icon against text. The spacing of token 2
is already the default.
<ButtonLink
// -- Other props -- //
leftIcon={
<Icon {...iconProps} />
}
>
Languages {i18n.language.toUpperCase()}
</ButtonLink>
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.
@rohan9024 this suggested change is still outstanding :)
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.
@TylerAPfledderer done✌
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.
Hey @rohan9024 I don't see that this was implemented. I think this change is important to simplify the overall code and to use the already provided Chakra api.
Marking it as unresolved for now.
src/components/Nav/index.tsx
Outdated
style={{ | ||
transition: "transform 0.3s ease-in-out, color 0.2s", // Apply the transition to the icon | ||
transform: languagesHover | ||
? "rotate(30deg)" | ||
: "rotate(0deg)", // Rotate the icon to 30 degrees on hover |
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.
There are a couple of pretty cool things to do here. Both transition
and transform
are style props, so they should not be under the style
prop.
Secondly, we can put transform
under the hover pseudo and not waste energy on useState
. This is a two step process.
- Add
data-group
toButtonLink
- In the
Icon
use the_groupHover
prop which will targetdata-group
with a descendant selector. so when hoveringButtonLink
the styles under this prop will trigger. Then no need for this ternary!
So in short...
<ButtonLink
// -- Other props -- //
data-group
leftIcon={
<Icon
as={MdLanguage}
transition="transform 0.3s ease-in-out, color 0.2s"
// _groupHover renders "[data-group]:hover &"
_groupHover={{
transform: "rotate(30deg)"
}}
/>
}
/>
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.
@rohan9024 marking this as unresolved.
Currently we have the _groupHover
but not the data-group
set in the parent ButtonLink.
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
Co-authored-by: Tyler Pfledderer <tyler.pfledderer@gmail.com>
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.
I missed one! :D
@TylerAPfledderer Thanks for providing your valuable suggestions😊. I have made the changes you've requested. Incase if I have missed out on something please guide me✌ |
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.
Hey @rohan9024 I've noticed that the PR is failing to build.
The error is that the ButtonLink
doesn't have an href
prop which is a required prop.
So, I ran this PR locally and fixed that to see the result and this is how I see the button now:
You can see that the icon is displayed as a button and that is a bit vertically misaligned.
As I said in the comment below, I think that by removing the nested ButtonLink
, that should fix these visual issues.
src/components/Nav/index.tsx
Outdated
<ButtonLink | ||
data-group | ||
leftIcon={ |
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.
Not sure if I follow what we are trying to do here but currently, you have a nested ButtonLink
inside another ButtonLink
, resulting in a nested a
tag.
I guess that what we would like to do here is to render just the icon, right? and add the data-group
tag on the parent ButtonLink
.
====
Note aside, the build is failing because this ButtonLink
is missing the href
prop.
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.
Yea, the example I gave was not suggesting nested, but a simplified version of what the component should look like with that data-*
prop and the leftIcon
prop.
It was only meant as a simplified example; not to actually simplify it that much! 😅
@pettinarip Alright got it! Updating the code..... |
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.
@rohan9024 I've added a few comments to make this cleaner. There are also 2 change requests added by Tyler that should be tackled as well.
Thanks for the time you have put in this. Let me know if you need help doing any of these changes or if I wasn't clear enough.
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
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.
Thanks @rohan9024
I committed a few changes to get this over the finish line:
- Took text out of a span, and added the translated template string back here (just reverted your change really around this)
- Added the leftIcon back in for the ButtonLink and removed the Icon you had added (reverted to how to use the ButtonLink component)
- Added animation to the icon for languages, which you can check out in the hover styles if interested.
- Removed unused useState code
Congrats, your important contribution to this open-source project has earned you a GitPOAP! Be sure to join the Ethereum.org discord if you are interested in contributing further to the project or have any questions for the team. GitPOAP: 2023 Ethereum.org Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
For context, refer this PR--> #10811
These were the changes requested-
Changes done by me-
Here is the working,
Home._.ethereum.org.-.Brave.2023-09-05.22-07-35.mp4