upgrade: fullscreen package upgrade for Solid 2.0#848
Conversation
🦋 Changeset detectedLatest commit: 23fcf5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
# Conflicts: # packages/utils/package.json # pnpm-lock.yaml
# Conflicts: # pnpm-lock.yaml
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/fullscreen-solid-2-migration.md:
- Around line 5-23: Update the migration note to match the shipped API: change
the example using the old ref usage so the Solid 2.0 example shows <div
ref={fullscreen()} /> (not fullscreen(fs)), replace mentions of the outdated
version tag `beta.7` with the actual shipped version if different, and update
the export signature description from `fullscreen(active?, options?)` to the
correct `fullscreen(options?)`; ensure you reference the helper factory name
`fullscreen` and the original `createFullscreen` in the prose so readers can map
the ref directive to the underlying API.
In `@packages/fullscreen/src/index.ts`:
- Around line 127-131: The current element binder overwrites removeListener
without removing any previously attached listener, risking orphaned click
handlers; inside the returned function (the callback that takes el: HTMLElement)
call the existing removeListener (if defined) before creating the new [enter,
exit] via makeFullscreen and binding toggle, then set removeListener to the new
cleanup function; reference the existing symbols removeListener, makeFullscreen,
enter, exit, and toggle when implementing this check and reassignment.
- Around line 90-96: When the ref changes you set bound (in the createEffect
callback) but don't resync the isActive state; after assigning bound = node ?
makeFullscreen(node, nativeOptions) : null you must call the same sync routine
that the 'fullscreenchange' listener uses to update isActive (e.g. invoke the
handler that updates the isActive signal or explicitly read the current state
from bound (bound?.isActive() or query document.fullscreenElement) and set the
isActive signal accordingly); update the createEffect callback (near ref,
createEffect, bound, makeFullscreen, nativeOptions, isActive, fullscreenchange)
to run that sync so isActive is correct immediately when ref swaps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e0acaf-30db-4a01-8a35-4052a9fa6736
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/fullscreen-solid-2-migration.mdpackages/fullscreen/README.mdpackages/fullscreen/package.jsonpackages/fullscreen/src/index.tspackages/fullscreen/test/index.test.tspackages/fullscreen/test/server.test.tspackages/fullscreen/test/setup.tspackages/fullscreen/tsconfig.json
Bumps the peer dependency to
solid-js@^2.0.0-betaand updates the package to match the 2.0 API.Breaking
use:createFullscreendirective removed — useref={fullscreen(opts)}insteadsolid-js@^2.0.0-betaand@solidjs/web@^2.0.0-betaChanges
fullscreen()ref factory export as the Solid 2.0 replacement for the directiveisServerimported from@solidjs/webcreateEffectupdated to the split compute/effect signatureSummary by CodeRabbit
Release Notes
Breaking Changes
refdirective.New Features
Documentation