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

VIDCS-3452: Clean up language for buttons in VERA #99

Merged
merged 15 commits into from
Feb 26, 2025

Conversation

cpettet
Copy link
Contributor

@cpettet cpettet commented Feb 25, 2025

What is this PR doing?

This is a ToggleButton. This is a Button. We don't have any ToggleButtons, but we had many Buttons named as ToggleButton. Let's standardize this.

How should this be manually tested?

  • CI passes
  • go through normal flow from landing to goodbye and ensure nothing breaks

What are the relevant tickets?

A maintainer will add this ticket number.

Resolves VIDCS-3452

Checklist

[✅] Branch is based on develop (not main).
[ ] 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?

Comment on lines -32 to -35
const handleClickAway = (event: MouseEvent | TouchEvent) => {
const target = event.target as HTMLElement;

if (isSmallViewport && !target.closest('#emoji-grid-toggle')) {
Copy link
Contributor Author

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 😂

Copy link
Collaborator

@v-kpheng v-kpheng left a comment

Choose a reason for hiding this comment

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

LGTM! 💪 🚀

Copy link
Collaborator

@maikthomas maikthomas left a 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
Copy link
Collaborator

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.

@dwivedisachin
Copy link
Collaborator

Tested LGTM!! 🚀

@cpettet cpettet merged commit 23a55ed into develop Feb 26, 2025
7 checks passed
@cpettet cpettet deleted the cpettet/vidcs-3452-renaming-buttons branch February 26, 2025 15:37
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.

4 participants