-
Notifications
You must be signed in to change notification settings - Fork 4
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
VIDCS-3452: Clean up language for buttons in VERA #99
Conversation
const handleClickAway = (event: MouseEvent | TouchEvent) => { | ||
const target = event.target as HTMLElement; | ||
|
||
if (isSmallViewport && !target.closest('#emoji-grid-toggle')) { |
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.
Missed during code review, but found during Cmd + F
- we don't actually have an id of emoji-grid-toggle
😂
…to cpettet/vidcs-3452-renaming-buttons
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.
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.
That's some honest work right here! LGTM
return; | ||
} | ||
// If a user clicks the toggle button, we save their preference for later. If a user doesn't select | ||
// If a user clicks the toggle button on mobile, we save their preference for later. If a user doesn't select |
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.
nit I don't think this is really a preference, I think you can just say that this will be handled by the Toolbar.
|
Tested LGTM!! 🚀 |
What is this PR doing?
This is a
ToggleButton
. This is aButton
. We don't have anyToggleButton
s, but we had manyButton
s named asToggleButton
. Let's standardize this.How should this be manually tested?
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDCS-3452
Checklist
[✅] Branch is based on
develop
(notmain
).[ ] Resolves a
Known Issue
.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md
?[ ] Resolves an item reported in
Issues
.If yes, which issue? Issue Number?