-
Notifications
You must be signed in to change notification settings - Fork 214
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
Rename mature
to includeSensitiveResults
#2721
Conversation
4c7516b
to
396309a
Compare
Size Change: +314 B (0%) Total Size: 871 kB
ℹ️ View Unchanged
|
Full-stack documentation: https://docs.openverse.org/_preview/2721 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. |
396309a
to
c84ce9a
Compare
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.
Are there any tests that we should be making locally? Is the app expected to behave as normal?
*/ | ||
const filterToString = (filterItem: FilterItem[]) => { | ||
const filterString = filterItem | ||
.filter((f) => f.checked) | ||
.map((filterItem) => filterItem.code) | ||
.join(",") | ||
return filterString === "mature" ? "true" : filterString | ||
// TODO: check this |
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.
Should this still be checked? 🙂
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.
Nope :D Removed the comment.
includeSensitiveResults: [ | ||
{ | ||
code: "includeSensitiveResults", | ||
name: "filters.mature.mature", |
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.
Does this name need to change as well?
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.
Since this PR does not change the string, it's better not to rename this key, too. Otherwise, we will make this string "fuzzy" in GlotPress - which is not really necessary just yet.
@@ -31,7 +32,7 @@ export const NoResultsTemplate = (args) => ({ | |||
<template #image> | |||
<VErrorImage :error-code="args.errorCode" /> | |||
</template> | |||
<VNoResults :type="args.type" :query="args.query"/> | |||
<VNoResults :media-type="args.type" :query="args.query" :search-term="args.query.q" :external-sources=[] /> |
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.
VNoResults
doesn't take query
as a prop.
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.
473ec94
to
59c36a0
Compare
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: Zack Krida <[email protected]>
Fixes
Fixes #2548 by @dhruvkb
Description
The
mature
query parameter is renamed tounstable__include_sensitive_results
.The filter category is renamed from
mature
toincludeSensitiveResults
The filter object in the search store is changed from
mature: { code: "mature", name: "...", checked: true }
toincludeSensitiveResults: { code: "includeSensitiveResults", name: "", checked: true }
.Testing Instructions
We have extensive unit tests for transforming the query to filters in the
search
store, and filters to the API query parameters. They should pass.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin