-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
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 well both on a large screen and on mobile.
The iframe title/ aria-label
can be fixed here or in another PR.
It looks great to me. Nice work |
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 great!
af989a7
to
5d4e46e
Compare
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/1325 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
Size Change: -439 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
5d4e46e
to
d8f8a7b
Compare
Fixes
Fixes #888 by @zackkrida
Description
Constrains the sketchfab viewer to the 1000px/500px (2/1) aspect ratio indicated by @panchovm in the comments in the linked issue.
Testing Instructions
Check out this branch and run the app using
pnpm dev
. Then visitlocalhost:8443/image/00a6a7a4-6605-4cba-bacf-a08b0da546b9
and verify that the viewer is the correct aspect ratio at various screen widths.I thought about adding a snapshot test for this but wasn't sure about it. Would we mock the request to the sketchfab-viewer script and the subsequent sketchfab API requests for the model information? Seems thorny!
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin