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

RadioGroupButton: VR motion implementation #3797

Merged
merged 7 commits into from
Oct 9, 2024
Merged

Conversation

diyorbek
Copy link
Contributor

@diyorbek diyorbek commented Oct 7, 2024

Summary

VR RadioGroupButton motion implementation WITHOUT other VR specific styles.

What changed?

BEFORE
ezgif-7-4b5c4062c9

AFTER
ezgif-1-a2e9bd38d7

Links

Checklist

  • Added unit tests
  • Added documentation + accessibility tests
  • Verified accessibility: keyboard & screen reader interaction
  • Checked dark mode, responsiveness, and right-to-left support
  • Checked stakeholder feedback (e.g. Gestalt designers, relevant feature teams)

@diyorbek diyorbek requested a review from a team as a code owner October 7, 2024 17:41
Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for gestalt ready!

Name Link
🔨 Latest commit 84a7884
🔍 Latest deploy log https://app.netlify.com/sites/gestalt/deploys/670670fd99143000099c8542
😎 Deploy Preview https://deploy-preview-3797--gestalt.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@diyorbek diyorbek added the minor release Minor release label Oct 7, 2024
@@ -186,7 +186,7 @@ const ButtonToggleWithForwardRef = forwardRef<HTMLButtonElement, Props>(function
const isDarkModeRed = isDarkMode && color === 'red';
const { isFocusVisible } = useFocusVisible();

const buttonToggleAnimation = useButtonToggleAnimation();
const buttonToggleAnimation = useTapScaleAnimation();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

just renaming i guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, i renamed it cuz now they is shared

const styleSize = size === 'sm' ? controlStyles.sizeSm : controlStyles.sizeMd;

const bgStyle = disabled && !checked ? styles.BgDisabled : styles.BgEnabled;

const { isFocusVisible } = useFocusVisible();

const { parentName } = useRadioGroupContext();

const tapScaleAnimation = useTapScaleAnimation();
Copy link
Contributor

@AlbertCarreras AlbertCarreras Oct 7, 2024

Choose a reason for hiding this comment

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

nit

const { handleMouseDown, handleMouseUp, elementRef } = useTapScaleAnimation();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't destructure the object to keeps its context "visible" while reading the code. We may have other even handler functions in the future. But this one is just for the animation. That's why i think its better to keep it this way.

@AlbertCarreras
Copy link
Contributor

i assume we don't need to listen to useReducedMotion here... can you confirm that?

@AlbertCarreras AlbertCarreras self-requested a review October 7, 2024 20:37
Copy link
Contributor

@AlbertCarreras AlbertCarreras left a comment

Choose a reason for hiding this comment

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

Few nits, but overall LGTM

@diyorbek
Copy link
Contributor Author

diyorbek commented Oct 8, 2024

i assume we don't need to listen to useReducedMotion here... can you confirm that?

I think so. There were no relevant specs.

@diyorbek diyorbek merged commit 6f6638e into master Oct 9, 2024
13 checks passed
@diyorbek diyorbek deleted the radiogroupbutton-motion branch October 9, 2024 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor release Minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants