-
Notifications
You must be signed in to change notification settings - Fork 63
Conversation
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1926 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: -7.47 kB (-1%) Total Size: 808 kB
ℹ️ View Unchanged
|
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.
No blocking comments, but 1 nit and a 1 question.
@@ -29,11 +32,11 @@ import { defineComponent } from '@nuxtjs/composition-api' | |||
import VLogoLoader from '~/components/VLogoLoader/VLogoLoader.vue' | |||
import VButton from '~/components/VButton.vue' | |||
|
|||
import OpenverseLogoText from '~/assets/icons/openverse-logo-text.svg?inline' | |||
import OpenverseLogoText from '~/assets/icons/openverse-logo-text.svg' |
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 don't remember if there was a specific reason we are getting rid of ?inline
. But this is leading to the SVG tag being duplicated in both the SVG file and in the HTML.
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 wrote about the reasons in this issue: https://github.com/WordPress/openverse-frontend/issues/1715.
I haven't noticed the duplication, I'll look into it, thank you!
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 don't see the SVG
tag duplication, @dhruvkb:
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @zackkrida Excluding weekend1 days, this PR was updated 5 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes |
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.
LGTM! Glad to see this one finally converted. Nice work getting through all of its dependencies 🥳
Fixes
Fixes #1925 by @obulat
Description
This PR converts JSDoc types to TS syntax, and adds the
VModal
directory totsconfig.json
Testing Instructions
The
types
step of CI should pass.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin