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: Issue 30 : fixes the checkbox animation on web export #31

Merged

Conversation

the-simian
Copy link
Contributor

This updates the Checkbox to do 3 small things:

  1. use a Forward Ref component on a View (not an SVG)
  2. the 'hook' approach' via useSharedValue and withTiming hooks
  3. animate opacity via the useSharedValue

This resolves #30

I also added a prettierrc.json at the root of the project that should match the existing code style (I noticed you favored single quotes over double, so this enforces that)

@the-simian
Copy link
Contributor Author

checkbox-animation-working-web-export

Copy link
Owner

@mrzachnugent mrzachnugent left a comment

Choose a reason for hiding this comment

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

@the-simian Thank you for contributing! I left some comments for next time. I see you put a good amount of effort. Here's the code I would use to update the component:

You can update this PR or I can add it, up to you

import { Check } from 'lucide-react-native';
import * as React from 'react';
import { Pressable } from 'react-native';
import Animated, { FadeIn } from 'react-native-reanimated';
import { cn } from '~/lib/utils';

interface CheckboxProps {
  value: boolean;
  onChange: (checked: boolean) => void;
  iconClass?: string;
  iconSize?: number;
}

const Checkbox = React.forwardRef<
  React.ElementRef<typeof Pressable>,
  Omit<React.ComponentPropsWithoutRef<typeof Pressable>, 'onPress'> &
    CheckboxProps
>(({ className, value, onChange, iconClass, iconSize = 16, ...props }, ref) => {
  return (
    <Pressable
      ref={ref}
      role='checkbox'
      accessibilityState={{ checked: value }}
      className={cn(
        'peer h-7 w-7 shrink-0 flex items-center bg-card justify-center rounded-md border border-primary web:ring-offset-background web:disabled:cursor-not-allowed web:disabled:opacity-50',
        className
      )}
      onPress={() => {
        onChange(!value);
      }}
      {...props}
    >
      {value && (
        <Animated.View entering={FadeIn.duration(200)}>
          <Check size={iconSize} className={cn('text-foreground', iconClass)} />
        </Animated.View>
      )}
    </Pressable>
  );
});

Checkbox.displayName = 'Checkbox';

export { Checkbox };

components/ui/checkbox.tsx Outdated Show resolved Hide resolved
components/ui/checkbox.tsx Outdated Show resolved Hide resolved
components/ui/checkbox.tsx Outdated Show resolved Hide resolved
components/ui/checkbox.tsx Outdated Show resolved Hide resolved
@the-simian
Copy link
Contributor Author

the-simian commented Feb 6, 2024

@mrzachnugent I tried your solution, but it seems to have a different issue than the original one I posted. Basically it doesn't actually "fade in" consistently. I noticed this in the existing version as well when I was working on it. This is why I did not use Animated.View with the entering and exiting props. They just weren't working as I'd expect

If you add an exit tween, it also does something very strange, where it slides off to the lower right corner. I've increased the times so that what I am seeing is more easy to catch with the naked eye:

    <Animated.View
          entering={FadeIn.duration(800)}
          exiting={FadeOut.duration(800)}
        >

using these speeds (which like I said are far too long and just you can see the problem in a gif)

checkbox-animation-not-working-web-export

In this gif I click the moment I enter the box. instead of a fade there's a wait and then a 'pop'. the exit animation (like it is in main) sometimes causes the checkbox to slide off to the side.

I think that the fact that its using value && means that the animation isn't reliably playing when the node is created. And since it is 'abruptly' destroyed it never played the exit animation correctly. You'd have to wait at least the length of the exit animation before removal to see the exit animation basically.

I don't think that the Animated.View itself is the issue, and I agree the way you're using it in your proposed changed is a better way forward than the Animated.createAnimatedComponent approach. Its much simpler.

I am going to try to adapt the version you have so that it uses opacity like in my proposed solution. I think that will solve the issue in the original ticket as well as play the tween correctly.

@the-simian
Copy link
Contributor Author

the-simian commented Feb 6, 2024

I've updated your solution in three ways

  1. I've removed value && . If you leave this there, the exit animation will not play.
  2. I've changed to use opacity rather than entering and exiting props.
  3. I've switched back to the sharedValue and withTiming but I'm using them in a way that is more consistent with their documentation. Thankfully since we're not doing 'computed values' I don't think a useDerived is necessary here, which is nice to keep it simpler.
  const opacity = useSharedValue(0);

  opacity.value = withTiming(value === true ? 1.0 : 0.0, {
    duration: 800, //I'm lowering this for the PR, but I made it long so you can see if the animation is really playing in the gif below
  });

long-so-you-can-see-it

I believe the way I am using these hooks is now how they are intended to be used, by updating the .value directly

you can see the useTiming example here

function App() {
  sv.value = withTiming(0);
  // ...
}

and useSharedValue here:

function App() {
  const sv = useSharedValue(100);

  // read a shared value
  console.log(sv.value);

  // and modify it
  sv.value += 50;
}

As for your other comments this PR avoids all your other concerns. Thank you for all your feedback. I feel much more confident about this approach. Its the best of all the above, and I think this one should be good to go.

@the-simian
Copy link
Contributor Author

the-simian commented Feb 6, 2024

to make merging easier, here it is working on web export and android in the most PR update:

web android
pr-web-working-2 image

I've changed duration back to 200. Its hard to see in the gif, but it is animating correctly/smoothly on all platforms ( i also tested it on ios)

Copy link
Contributor Author

@the-simian the-simian left a comment

Choose a reason for hiding this comment

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

I've resolved all your line item review concerns and am ready for you to re-review this version

Copy link
Owner

@mrzachnugent mrzachnugent left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

@mrzachnugent mrzachnugent merged commit e7ad16a into mrzachnugent:main Feb 7, 2024
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.

Checkbox Component: Uncaught TypeError: element.cloneNode is not a function
2 participants