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

Using Controlled zoom & ZoomContent at the same time breaks #448

Open
1 of 4 tasks
tarngerine opened this issue Nov 2, 2023 · 10 comments
Open
1 of 4 tasks

Using Controlled zoom & ZoomContent at the same time breaks #448

tarngerine opened this issue Nov 2, 2023 · 10 comments
Assignees
Labels

Comments

@tarngerine
Copy link

tarngerine commented Nov 2, 2023

Issue Type

  • Bug report
  • Feature request
  • Question / support request
  • Other

Description

See repro in Codesandbox link: https://codesandbox.io/s/snowy-hill-ltjwwc?file=/src/App.tsx

Using Controlled with ZoomContent prop breaks the webpage that it's used in. Using them separately works fine.

After the first zoom-in, the first zoom-out breaks (no animation), and every interaction on the website thereafter is locked.

@rpearce
Copy link
Owner

rpearce commented Nov 3, 2023

That sounds rough. Thanks for the report! I'll see what I can do.

@rpearce rpearce self-assigned this Nov 12, 2023
@rpearce rpearce added the bug label Nov 12, 2023
@rpearce
Copy link
Owner

rpearce commented Dec 15, 2023

This is the next issue I'm doing! Time-expectation-wise, I'll be able to look at it within the next week.

Thank you for your patience.

@rpearce
Copy link
Owner

rpearce commented Dec 20, 2023

Okay... initial finding:

Converting this

ZoomContent={(props) => <CustomZoomContent {...props} />}

to this

ZoomContent={CustomZoomContent}

works with the Controlled zoom.

However, the Uncontrolled component doesn't have this limitation. Let's see...

@rpearce
Copy link
Owner

rpearce commented Dec 20, 2023

I also was able to get it working using React.useCallback:

  const Zoom2Content = useCallback(
    (props: ControlledProps) => <CustomZoomContent {...props} />,
    []
  );

You can see that working on this CodeSandbox

@rpearce
Copy link
Owner

rpearce commented Dec 20, 2023

I think that the issue is that ={() => <Something />} creates a new instance of that component on every render, but this might not work for a reason I haven't figured out yet.

When we use Uncontrolled (the default component), it works, for (props) => <CustomZoomContent {...props} /> has only been called once, and Uncontrolled manages its own zoomed/unzoomed state.

However, with the Controlled example, if we update the parent component state, then a new instance of () => <Something /> is created and passed on every render, and the ZoomContent property seems like it can't handle that. Maybe it should?

There are some workarounds I've listed above, and this seems like a bit of a deep dive to figure out any further that I don't have time for tonight, unfortunately. G'night!

@firatciftci
Copy link

@rpearce The solution/workaround above with useCallback() works perfectly for my needs! I was struggling with this issue until I stumbled upon this thread, so I would recommend adding this as a suggested method (or a fine workaround) to the main README file. Thank you for all of your work on this!

@firatciftci
Copy link

firatciftci commented Jun 14, 2024

And separately, I had to get the internal props of ZoomContent copied over from the respective node_modules type definition file to set up the useCallback()-ed custom component as such:

const enum ModalState {
  LOADED = "LOADED",
  LOADING = "LOADING",
  UNLOADED = "UNLOADED",
  UNLOADING = "UNLOADING",
}

type ZoomContentProps = {
  img: React.ReactElement | null;
  buttonUnzoom: React.ReactElement<HTMLButtonElement>;
  modalState: ModalState;
  onUnzoom: () => void;
};

const ZoomContent = useCallback(
    (props: ZoomContentProps) => <CustomZoomContent {...props} />,
    [],
);

Exposing these type definitions from the package to the user to make this a bit easier (and less copy-paste-y) would perhaps be a good idea as well.

@rpearce
Copy link
Owner

rpearce commented Jun 14, 2024

I need to fix the core issue, and if there are more types that need exporting, I'll see to that.

Thanks for your notes.

@ScreamZ
Copy link

ScreamZ commented Jul 19, 2024

@rpearce Any news on that

@rpearce
Copy link
Owner

rpearce commented Jul 20, 2024

@ScreamZ The issue will be closed and commented on with the fix version when it's resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants