-
Notifications
You must be signed in to change notification settings - Fork 521
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 #4764: Spotlight overlays have some UI issues #4763
Fix #4764: Spotlight overlays have some UI issues #4763
Conversation
Update: All 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.
Thanks @JishnuGoyal! I think it looks much better! I have a few comments, but besides that the alignment of text looks a bit off, ditto for the new x button: can we keep the top of the x button aligned with the text? Also, the text top and bottom margins relative to the background should always be the same (sometimes it seems like one is larger than the other in your pictures).
Unassigning @BenHenning since the review is done. |
Hi @JishnuGoyal, it looks like some changes were requested on this pull request by @BenHenning. PTAL. Thanks! |
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.
Awesome work @JishnuGoyal Thanks.
Thanks! @rt4914 and @BenHenning. FYI i have updated the UI as requested and addressed the review comments. This PR is getting a bit time sensitive so i have updated everything to the best of my knowledge. PTAL. |
#4763 (comment) #4763 (comment) |
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 @JishnuGoyal. Just two follow-ups--PTAL. Otherwise, the PR 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.
Thanks, LGTM!
Unassigning @BenHenning since they have already approved the PR. |
Explanation
fixes #4764
Updated video demo:
spotlightdemo.mp4
Updated screenshots:
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Portrait
Land:
mobile rtl:
Tablet:
Video (spotlight experience):
WhatsApp.Video.2022-11-25.at.10.58.53.PM.mp4
Video (Talkback):
WhatsApp.Video.2022-11-25.at.11.02.41.PM.mp4