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

refactor(popper): Update to use react portal as default #545

Closed
wants to merge 4 commits into from
Closed

refactor(popper): Update to use react portal as default #545

wants to merge 4 commits into from

Conversation

mannycarrera4
Copy link
Contributor

Summary

Fixes: #517

Checklist

  • branch has been rebased on the latest master commit
  • tests are changed or added
  • yarn test passes
  • all (dev)dependencies that the module needs is added to its package.json
  • code has been documented and, if applicable, usage described in README.md
  • module has been added to canvas-kit-react and/or canvas-kit-css universal modules, if
    applicable
  • design approved final implementation
  • a11y approved final implementation
  • code adheres to the API & Pattern guidelines

Additional References

@anicholls
Copy link
Contributor

@mannycarrera4 Can you make sure you target prerelease/v4? Some of these updates (i.e. Popper) have already been made there

@mannycarrera4 mannycarrera4 changed the base branch from master to prerelease/v4 April 1, 2020 17:35
@cypress
Copy link

cypress bot commented Apr 1, 2020



Test summary

206 0 0 0


Run details

Project canvas-kit
Status Passed
Commit 1390a19 ℹ️
Started Apr 2, 2020 9:19 PM
Ended Apr 2, 2020 9:21 PM
Duration 02:16 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@mannycarrera4 mannycarrera4 added 4.x enhancement New feature or request labels Apr 1, 2020
@anicholls anicholls changed the title chore: Update components to use react portal as default refactor: Update components to use react portal as default Apr 2, 2020
@anicholls anicholls changed the title refactor: Update components to use react portal as default refactor(popper): Update to use react portal as default Apr 2, 2020
@NicholasBoll
Copy link
Member

I read the PR description and associated issue. We are already defaulting to using portals. This PR removes the option to not use a Portal. Is that what we wanted? There may be cases where not portalling content is desired.

@mannycarrera4
Copy link
Contributor Author

@NicholasBoll from what I understood, we were just using portals, but this brings up the question of if we should keep the portaling option if they don't want to portal?

@NicholasBoll
Copy link
Member

@NicholasBoll from what I understood, we were just using portals, but this brings up the question of if we should keep the portaling option if they don't want to portal?

I'm not sure what that means. By default we use portals with the option to disable if necessary (in certain cases this can make focus management much easier).

This API change still allows the original functionality of disabling the separation of child content from the parent (what portalling essentially does) by allowing a developer to set the containerElement to be the parent element, but this is much harder to do now:

// before
<Popper portal={false}>

// after
<Portal containerElement={targetRef.current?.parentElement}

Full example here:

// before
const BeforeComponent = () => {
  const targetRef = React.useRef<HTMLDivElement>(null)
  const [isOpen, setOpen] = React.useState(false)

  return (
    <>
      <div ref={targetRef} onClick={() => setOpen(true)}>Open</div>
      <Popper open={isOpen} anchorElement={targetRef.current} portal={false}>
        Popper content
      </Popper>
    </>
  )
}

// after
const AfterComponent = () => {
  const targetRef = React.useRef<HTMLDivElement>(null)
  const [isOpen, setOpen] = React.useState(false)

  return (
    <>
      <div ref={targetRef} onClick={() => setOpen(true)}>Open</div>
      <Popper open={isOpen} anchorElement={targetRef.current} containerElement={targetRef.current?.parentElement}>
        Popper content
      </Popper>
    </>
  )
}

The after isn't 100% accurate because we can no longer control the position in the DOM if we don't want to portal to a separate container. Portals will always be a the last child of a container.

@NicholasBoll
Copy link
Member

@anicholls Is this an okay thing? No way to disable the use of Portal on a Popper component? There are valid reasons to not use it, but still have the niceties of using a position library.

@mannycarrera4
Copy link
Contributor Author

@NicholasBoll I might have misinterpreted the issue, i think after your explanation on the example, i think we should just default to using portal instead of ripping it out

@mannycarrera4
Copy link
Contributor Author

@anicholls @NicholasBoll I feel like we should just close this PR, most of the changes where in popper and they're being addressed in #528 thoughts? I only really just updated some readmes

@anicholls
Copy link
Contributor

@mannycarrera4 I'm fine with closing this in favor of #528.

@NicholasBoll Can you make sure #517 gets closed as part of #528 as well? Squashing 10 tickets with one PR 😂

@anicholls anicholls closed this Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default our Popups to use React Portal
3 participants