-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
feat: update extended client:visible to use an object instead of a string #9596
Conversation
🦋 Changeset detectedLatest commit: 8cbb65d The changes in this PR will be included in the next version bump. 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 |
⚖️ Bundle Size CheckLatest commit: 8cbb65d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Can we add a test that these arguments are properly encoded in the HTML?
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test! Thank you!
packages/astro/e2e/fixtures/custom-client-directives/client-options.js
Outdated
Show resolved
Hide resolved
const ioOptions = { | ||
rootMargin: typeof options.value === 'string' ? options.value : undefined, | ||
const rawOptions = | ||
typeof options.value === 'object' ? (options.value as ClientVisibleOptions) : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final nit: this technically adds support for all IntersectionObserverInit
values, but our types are narrowed to only support Pick<IntersectionObserverInit, 'rootMargin'>
. Is that desired?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, see the code below
Changes
Revert #9363 and redo it using an object instead of a string, that way it can supports options we'll add in the future.
Testing
Tested manually, like the original PR.
Docs
Will update docs!