-
Notifications
You must be signed in to change notification settings - Fork 73
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(weave): Add support for jpegs and pngs #3304
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=97b202171313ab6c9d1f1f738f697b254b385467 |
e24aef5
to
e0a1b4e
Compare
.../src/components/PagePanelComponents/Home/Browse3/typeViews/PIL.Image.Image/PILImageImage.tsx
Outdated
Show resolved
Hide resolved
.../src/components/PagePanelComponents/Home/Browse3/typeViews/PIL.Image.Image/PILImageImage.tsx
Outdated
Show resolved
Hide resolved
key => key in imageTypes | ||
) as keyof PILImageImageTypePayload['files']; | ||
if (!imageKey) { | ||
throw new Error('No image file found'); |
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.
def don't throw. just render a empty / some info state. Throwing will crash the page
key => key in imageTypes | ||
) as keyof PILImageImageTypePayload['files']; | ||
if (!imageKey) { | ||
return <span>Img not found!</span>; |
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 would use weave-js/src/components/PagePanelComponents/Home/Browse2/NotApplicable.tsx
this component
key => key in imageTypes | ||
) as keyof PILImageImageTypePayload['files']; | ||
if (!imageKey) { | ||
return <span>Img not found!</span>; |
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.
early returns must happen after all use hooks
|
||
const imageKey = Object.keys(props.data.files).find( | ||
key => key in imageTypes | ||
) as keyof PILImageImageTypePayload['files']; |
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.
This is still not correct since you are not guaranteed to find the key. This cast is invalid and causes line 37 to be incorrect.
Resolves:
This PR adds support for logging and viewing JPEGs. Previously, all images would be saved as PNG, even if the source image was a JPEG.
If you somehow force-logged a JPEG, it still wouldn't work because the UI didn't support JPEGs.
(before)
(after)
Testing is mostly manual, with some client tests added for the different formats. We should add playwright tests when they are available. I have verified that the image that comes back is a jpeg (and you can confirm by inspecting/saving the img)