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

fix: dancing shortcut popup #3697

Merged
merged 10 commits into from
Oct 30, 2024
Merged

Conversation

Abhi-Bohora
Copy link
Contributor

@Abhi-Bohora Abhi-Bohora commented Oct 21, 2024

Changes

Linked Issue: (dailydotdev/daily#1357)

Describe what this PR does

This pr aims to fix the shortcut link input box UI glitch in the extension of daily.dev as shown in the video in the linked issue whenever users try to input a shortcut link and enter an invalid URL the input box begins to glitch up and down infinitely. I think this was caused by a condition check in a useEffect that created an infinite loop.

note: I have tested this only in Chrome I believe it should work for another browser extension too.

After changes

Untitled.video.-.Made.with.Clipchamp.mp4

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Copy link

vercel bot commented Oct 21, 2024

@Abhi-Bohora is attempting to deploy a commit to the Daily Dev Team on Vercel.

A member of the Team first needs to authorize it.

@Abhi-Bohora
Copy link
Contributor Author

Abhi-Bohora commented Oct 23, 2024

@sshanzel I have kept all the old code implementations as they are turns out it was just an if condition in useEffect that was causing this issue. Thanks for your response; I was going down the wrong path before. I have built and tested this.

@sshanzel
Copy link
Member

@sshanzel I have kept all the old code implementations as they are turns out it was just an if condition in useEffect that was causing this issue. Thanks for your response; I was going down the wrong path before. I have built and tested this.

Great to see you found an alternative. This is also a heavily used hook, seeing how the changes turned out to be simpler would make testing easier.

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Really sorry that it took a while to send a review on this. Kindly check my message about the removed condition.

@Abhi-Bohora
Copy link
Contributor Author

@sshanzel thanks for the review, I hope this was the solution you are looking for

Copy link
Member

@sshanzel sshanzel left a comment

Choose a reason for hiding this comment

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

Yes! This is exactly it. Thank you so much for your effort. All what is left now is for me to test 🙏

@sshanzel
Copy link
Member

Tested it and indeed the ref worked as intended compared to without it 🤩

Copy link

vercel bot commented Oct 30, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Oct 30, 2024 7:11am

@sshanzel sshanzel merged commit 9ac4a33 into dailydotdev:main Oct 30, 2024
8 of 9 checks passed
@Abhi-Bohora Abhi-Bohora deleted the dancing-shortcut branch October 30, 2024 10:23
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.

2 participants