-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
[Avatar] Support cross origin in useImageLoadingStatus #1433
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Thanks for your contribution!
I added a few comments. Could you also run pnpm proptypes
and commit the changed files after you resolve them so proptypes definitions are generated?
My bad, I didn't even notice the proptypes. 😅 I think that should be all, do let me know if there's anything else. Thanks! |
} | ||
if (options?.crossOrigin) { | ||
image.crossOrigin = options.crossOrigin; | ||
if (typeof crossOrigin === 'string') { |
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.
Is this check necessary? If crossOrigin
is undefined, it'll be passed as such to image
and it should still work, no?
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.
Well, according to MDM CrossOrigin Docs:
The
crossorigin
content attribute on media elements is a CORS settings attribute.These attributes are enumerated, and have the following possible values:
anonymous
Request uses CORS headers and credentials flag is set to 'same-origin'. There is no exchange of user credentials via cookies, client-side TLS certificates or HTTP authentication, unless destination is the same origin.
use-credentials
Request uses CORS headers, credentials flag is set to 'include' and user credentials are always included.
""
Setting the attribute name to an empty value, like crossorigin or crossorigin="", is the same as anonymous.An invalid keyword and an empty string will be handled as the anonymous keyword.
Assuming that an image is loading from a cors server, without the image.crossOrigin
set, the image.src
will hit a cors error, then image.onload
will never be called. Which mean the image won't render. So by checking if it's a string type (empty string/invalid strings), this way we can just treat it as how MDM defined crossOrigin.
Hopefully that make sense.
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.
In short, we would want to make sure ""
is assigned to image.crossOrigin
.
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.
I meant if an invalid or undefined crossOrigin
is passed to AvatarImage, it should be forwarded as-is to useImageLoadingStatus
. By adding if (typeof crossOrigin === 'string') {
, we're actually changing the behavior described in MDN, because if someone set <AvatarImage crossOrigin={42}>
, this implementation would not set crossOrigin
on an image
at all, whereas, according to MDN, it should treat it as "anonymous"
.
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.
Yes, you're right. Fixed it. Had to do an nullish check since image.crossOrigin
doesn't accept an undefined type.
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.
Thanks, it looks good now!
Description
Add crossOrigin Support to useImageLoadingStatus Hook.
Note: radix-ui/primitives(https://github.com/radix-ui/primitives) has this exact issue and already has a PR radix-ui/primitives#3261. So here's a corresponding PR for Base UI 😊