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

React DOM: Add support for Popover API #27981

Merged
merged 7 commits into from
May 20, 2024
Merged

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jan 17, 2024

Summary

Closes #27479
Alternate to #27480

This includes support for popover, popoverTarget, popoverTargetAction, onToggle and onBeforeToggle. The events are only triggered for elements that have a popover prop matching the spec.

The Popover API is currently supported in all major browsers (Chrome, Safari, Firefox)

We decided to go with popoverTarget instead of popoverTargetElement for the prop naming since popoverTarget takes string just like popovertarget does. popoverTargetElement reads and writes HTMLElement. popoverTarget={object} will warn unlike other props since we'll overload this prop to accept DOM Elements in the future.

How did you test this change?

  • Attribute fixture (commited changes with non-experimental build since this is the build we were using before)
  • Codesandbox supports basic Popover functionality

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 17, 2024
@react-sizebot
Copy link

react-sizebot commented Jan 17, 2024

Comparing: 1d6eebf...0850281

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.14% 495.01 kB 495.71 kB +0.12% 88.68 kB 88.78 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.14% 499.81 kB 500.51 kB +0.13% 89.36 kB 89.47 kB
facebook-www/ReactDOM-prod.classic.js +0.12% 592.16 kB 592.86 kB +0.12% 104.15 kB 104.28 kB
facebook-www/ReactDOM-prod.modern.js +0.12% 568.39 kB 569.08 kB +0.15% 100.55 kB 100.70 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactTestUtils-dev.classic.js +0.34% 44.53 kB 44.68 kB +0.24% 12.75 kB 12.78 kB
facebook-www/ReactTestUtils-dev.modern.js +0.34% 44.53 kB 44.68 kB +0.24% 12.75 kB 12.78 kB
test_utils/ReactAllWarnings.js Deleted 64.26 kB 0.00 kB Deleted 16.02 kB 0.00 kB

Generated by 🚫 dangerJS against 0850281

@eps1lon eps1lon marked this pull request as ready for review January 17, 2024 17:29
@acusti
Copy link
Contributor

acusti commented Jan 17, 2024

@eps1lon first of all, can’t wait for this to land! the browser console and linting errors are distracting and painful. i asked this on the previous PR but never heard a reply, so trying again here: do you know how much of a lift it would be to extend support to include the new toggle and beforetoggle events associated with the Popover API?

@eps1lon
Copy link
Collaborator Author

eps1lon commented Feb 18, 2024

@acusti Thank you for raising this. These events should be part of this PR. I'll add some tests.

@eps1lon eps1lon marked this pull request as draft February 18, 2024 15:29
@acusti
Copy link
Contributor

acusti commented Apr 21, 2024

@eps1lon i just tried out this build (based on your instructions, thank you) and it’s looking great! way better to be able to use onToggle={handleToggle} and not a bunch of nonsense with refs / state / effects to (add|remove)EventListener during the component lifecycle.

one thing i noticed is that the ToggleEvent’s instance properties newState and oldState aren’t available on the SyntheticBaseEvent event that is received when using the onToggle prop to assign a handler. the instance properties are available on the event.nativeEvent, so i’m not blocked, but it surprised me.

i will be using this build and will report if anything else comes up, but based on using it so far, i’m very excited for this support to land. and for anyone following this PR, you can try out the builds from this PR by installing react and react-dom using these identifiers:

react@https://react-builds.vercel.app/api/prs/27981/packages/react react-dom@https://react-builds.vercel.app/api/prs/27981/packages/react-dom

UPDATE see below for latest dependency strings

@mariusGundersen
Copy link

Will this be part of react 19?

@eps1lon eps1lon force-pushed the feat/popover-api branch 3 times, most recently from 83bcaab to 29b66a5 Compare May 1, 2024 13:52
@eps1lon
Copy link
Collaborator Author

eps1lon commented May 1, 2024

@acusti Good catch, thank you. Should work with the latest version of this branch: https://codesandbox.io/p/github/eps1lon/react-popover-api-demo/main

@acusti
Copy link
Contributor

acusti commented May 1, 2024

@eps1lon i just tried it out using the dependency strings from your codesandbox and the toggle event newState / oldState properties are there and correct 🎉

react@https://react-builds.vercel.app/api/commits/29b66a538dfca92616c145c811f810ca2987ae29/packages/react react-dom@https://react-builds.vercel.app/api/commits/29b66a538dfca92616c145c811f810ca2987ae29/packages/react-dom

UPDATE see below for latest dependency strings

Copy link

vercel bot commented May 17, 2024

@eps1lon is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 17, 2024

We decided to go with popoverTarget instead of popoverTargetElement for the prop naming since popoverTarget takes a string just like popovertarget does. popoverTargetElement reads and writes HTMLElement.

@eps1lon eps1lon force-pushed the feat/popover-api branch from 5d43e38 to 65999a0 Compare May 17, 2024 19:03
Copy link

vercel bot commented May 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 7:10pm

@acusti
Copy link
Contributor

acusti commented May 19, 2024

dependency strings for those who want to try out the latest version of support (rebased off of latest react/react-dom and including the prop name change reversion back from popoverTargetElementpopoverTarget):

react@https://react-builds.vercel.app/api/commits/f74fb57f87832af99696214075e48246018678ec/packages/react react-dom@https://react-builds.vercel.app/api/commits/f74fb57f87832af99696214075e48246018678ec/packages/react-dom

@eps1lon eps1lon merged commit 6f90365 into facebook:main May 20, 2024
40 checks passed
github-actions bot pushed a commit that referenced this pull request May 20, 2024
@eps1lon eps1lon deleted the feat/popover-api branch May 20, 2024 20:27
@weilbith
Copy link

I'm highly interested in this feature. Is it worth to wait, because it will just take a few days till a new release or...? No idea about the release process of React. But I really wanna use this in combination with a polyfill where needed. Would be a bummer if that's not possible. 😕

@acusti
Copy link
Contributor

acusti commented May 25, 2024

@weilbith if you use the latest react RC, all popover API attributes and event handlers are available and working. i'm using the following versions (prepend the code with npm i or yarn add to upgrade to this version):

react@19.0.0-rc-4c2e457c7c-20240522 react-dom@19.0.0-rc-4c2e457c7c-20240522

@mariusGundersen yes! it's landed in the latest react v19 RC as of the version string above.

@acusti
Copy link
Contributor

acusti commented May 26, 2024

one last note for @weilbith and @mariusGundersen and anyone else trying to use React’s popover API support in a typescript project. you will need to use the latest version of @types/react (18.3.3) and enable the react/canary types, which you can do so by including /// <reference types="react/canary" /> in any file in your project (more details here).

@peteruithoven
Copy link

Is there any change the Popover API can be backported to v18?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Popover API not supported
9 participants