-
Notifications
You must be signed in to change notification settings - Fork 218
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
Add VIEW_EXTERNAL_SOURCES event #2450
Conversation
Size Change: +440 B (0%) Total Size: 830 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.
LGTM, except for a possible typo in the comment. Unblocking because it could very well be a misunderstanding on my part.
@@ -139,6 +146,11 @@ export default defineComponent({ | |||
|
|||
const isVisible = ref(false) | |||
|
|||
// Use the `_searchType` from mediaStore because it falls back to ALL_MEDIA |
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.
The comment says something different from what's happening. searhType
is being used from the searchStore
on L169.
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. The only requested change is to add resultPage
to the analytics type.
Note: types aren't failing on this PR because VExternalSearchForm
isn't type checked (not included in tsconfig.json
).
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.
After adding @sarayourfriend's comments, we can add VExternalSources
folder to tsconfig
. The types will already be correct, no other changes will be necessary:
"src/components/VExternalSearch/**.vue",
@obulat I tried adding the file to the tsconfig, but curiously the whole Vue component is underlined red with the following type error:
|
The fix to the type signature was made and the irrelevant comment was removed. |
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!
Co-authored-by: Olga Bulat <obulat@gmail.com>
Fixes
Fixes #1075 by @dhruvkb
Description
Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin