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
Extract filter definition to constants directory #1038
Conversation
d623755
to
05b4a1a
Compare
@@ -178,7 +87,7 @@ export const state = () => ({ | |||
size: '', | |||
source: '', | |||
searchBy: '', | |||
mature: false, | |||
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.
What if the mature
filter used the strings 'true' and 'false', so that we wouldn't need any custom logic for it in the search query transform functions?
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 think it would mean that we would have to move the custom logic somewhere else. Because there would have to be the logic of removing 'false' from the API query. Or do we want to send 'mature=false' with every query, instead of only adding 'mature=true' to the queries that set mature to true?
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.
ah yes, that's a good point. But would using '' and 'true' fix that?
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.
export const filtersToQueryData = ( |
In search-query-transform, above, we use the
query
object from the search store, and convert it into the the object that is used to create the API request URL. There, we use the hideEmpty
parameter set to true, so any query parameter that is equal to ''
is discarded, and non-blank parameters are used in the query string.
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.
So, in short, mature: 'anykindofstring'
is converted to /?q=cat&mature=anykindofstring
, but mature: ''
is converted to ?q=cat
.
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.
Yes, that's what I thought, thanks! So we could use '' for the default, off value of the mature filter, and the string 'true' as the value for showing mature images, which would allow us to avoid any custom conditionals for the mature filter in our search query transformation logic
LGTM! I have one question left about the mature filter, but it's a minor improvement.
This looks really great! I tested it locally and operationally it seems perfect. I tried a bunch of different filter scenarios and all of them worked.
Just noticed a couple things with the types, in particular with the initFilters
type, that we should fix before merging. After that this is G2G from my perspective.
src/constants/filters.ts
Outdated
* A list of filters that are only used for the specific content type. | ||
* This is used to clear filters from other content types when changing the content type. | ||
*/ | ||
export const mediaUniqueFilterKeys = deepFreeze<Record<SearchType, string[]>>({ |
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 these also filter types maybe?
export const mediaUniqueFilterKeys = deepFreeze<Record<SearchType, string[]>>({ | |
export const mediaUniqueFilterKeys = deepFreeze<Record<SearchType, FilterType[]>>({ |
@@ -47,7 +47,7 @@ export type Query = { | |||
categories: string | |||
source: string | |||
duration: string | |||
mature: boolean | |||
mature: string |
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.
Makes me want to change all these string
types to specific value types for each of them.
src/store/types.ts
Outdated
@@ -130,6 +130,7 @@ export interface Filters { | |||
searchBy: FilterItem[] | |||
mature: boolean | |||
} | |||
export type FilterType = keyof Filters |
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 Type
suffix is kind of confusing here and generally best avoided when naming types due to the ambiguity of it... would FilterCategory
be clearer (and also accurate)?
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 have not realized that it's the name of the type that ends with Type
:) Name updated.
tsconfig.json
Outdated
@@ -43,6 +43,7 @@ | |||
"src/composables/window.js", | |||
"src/constants/media.js", | |||
"src/constants/screens.js", | |||
"src/constants/filters", |
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 can be left out, all .ts
files are automatically included in the tsconfig scope.
Also for future reference these need to have the extension as well or the file won't get picked up.
"src/constants/filters", |
src/constants/filters.ts
Outdated
checked: false, | ||
})), | ||
}), | ||
{} |
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.
So... the Array.prototype.reduce
function's type inference is a little weird. It'll cast the return type as the inferred type of the initial value parameter, so we need to either make sure that the initial value parameter has the right type to begin with, or (in this and most cases) cast it to the correct type.
{} | |
{} as Record<FilterType, FilterItem[]> |
Currently initFilters
has the type () => {}
(i.e., returning an empty object literal). So filterData
is Readonly<{}>
. We're not seeing errors now because when we're using it elsewhere it's not being type-checked, but once we start using it in other type checked files it'll start complaining about this (probably with slightly confusing errors too unfortunately) so best to fix it now before it bites us down the line.
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.
TIL :) Thank you!
Fixes
Related to #1037 by @obulat
This is the first of 3 search store conversion PRs, and should be reviewed first.
Description
This PR extracts the filter creation from the search store file into a separate file in the constants module. I'm not sure this is the best location: we do create constants for initial filters here, but we use some functions to do that.
Other small changes in this PR:
[{ code: 'mature', name: 'the i18n key', checked: false }]
to match the other filters.searchType
for the search store toALL_MEDIA
.Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin