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.

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

fixes #428: Adding hover effect on the buttons #435

Closed

Conversation

VivekJaiswal18
Copy link
Contributor

What kind of change does this PR introduce?

feature fix

Issue Number:

Summary

Added hover effect to the buttons
Issue link: #428
Does this PR introduce a breaking change?

No

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Looks great in desktop but the effects are not working in mobile when I try it in the developer tools of my chrome browser. Not sure if it is just me. Can you please check?

Left a comment because of the unexpected change in package.json.

@@ -33,8 +33,8 @@
"next-plausible": "^3.11.1",
"next-sitemap": "^4.2.3",
"node-ical": "0.16.1",
"react": "18.2.0",
"react-dom": "18.2.0",
"react": "^18.2.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@VivekJaiswal18 Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjagm I introduced this change so that if minor patches were introduced by the library then anyone installing it can benefit from it (if anyone does npm install react/react-dom).
But if you think it should not be done then I can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

I think its good to remove that change since it also modifies files like package-lock.json and yarn.lock and ultimately increases the PR size.

image

Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

It will be a good enhancement if you reduce the duration time to 3ms with the help of duration-300

@DhairyaMajmudar
Copy link
Member

I checked the changes in responsive mode using Firefox and chrome dev tools. I found that its working fine if touch simulation mode is turned off.

@benjagm
Copy link
Collaborator

benjagm commented Mar 5, 2024

@VivekJaiswal18 Can you please take a look to the feedback provided in the review? Before merge you'll need to fix the merge conflicts as well.

@DhairyaMajmudar
Copy link
Member

@VivekJaiswal18 if you need any help feel free to ask : )

@VivekJaiswal18
Copy link
Contributor Author

@benjagm

@VivekJaiswal18 Can you please take a look to the feedback provided in the review? Before merge you'll need to fix the merge conflicts as well.

Okay @benjagm looking into it..sorry for the delay!

VivekJaiswal18 and others added 3 commits March 9, 2024 04:47
removed "block" from the classname

Co-authored-by: Dhairya Majmudar <[email protected]>
Co-authored-by: Dhairya Majmudar <[email protected]>
@VivekJaiswal18
Copy link
Contributor Author

PR for this issue is provided in #497 considering the Feedback provided in the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Enhancement: Adding hover effect on the buttons.
3 participants