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

chore: migrate IFrame component to functional component #21677

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

crutchcorn
Copy link

@crutchcorn crutchcorn commented Nov 21, 2024

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn lint)
    • Neither of these commands ran any tasks and npx nx graph doesn't show any task graphs either, I wonder if Nx is misconfigured.

We appreciate your contribution!


While looking at the codebase, I found:

  • An unmemoized head element that, on re-render, would wreck havoc on performance thanks to reconstructing all of the CSS styles
  • A class component that referenced migration to a functional component
  • Missing property typings for the iframe component

I figured I'd see if I could help with these issues, so I took a stab at it. I loaded the signup page and did a comparison against both the production tab and the development tab without my changes and it seemed to work fine.

I still worry about some of the uncleaned up effects. I know the comment mentions that the iframe being removed will cleanup the effects, but I'm not entirely certain of this. After all, we're passing an arrow function which the browser's GC might not handle properly.

This isn't a huge deal given the scope of this component, but it's a minimal refactor to handle in a more 'tidy' manner. I could do this refactor as a follow-up if requested.

Comment on lines +22 to +23
setIframeHead(node.current.contentDocument.head);
setIframeRoot(iframeRootLocal);
Copy link
Author

Choose a reason for hiding this comment

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

No need to recall this.forceUpdate anymore, since the batched updates will trigger a re-render, updating the iframeHead and iframeRoot usages in the JSX

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.

1 participant