-
-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: add search params in url #142
feat: add search params in url #142
Conversation
b4eb889
to
f43e759
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.
Thanks for the work @Kout95
-
Can we eventually split the history / params methods in a different file to avoid cluttering too much the search-bar. (at least the one doing transformations)
-
the prefix is not a constant but the search-bar name property
-
triggering first search should be done in a more reliable way
81a626d
to
0a51cb9
Compare
e5c7f28
to
7d053cc
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.
I think you're not ready with this one yet, right ? (for example, I still see OFF_PREFIX used)
bd845b7
to
1d6647c
Compare
Co-authored-by: Alex Garel <[email protected]>
…rectly in constructor
Co-authored-by: Alex Garel <[email protected]>
1d6647c
to
d7a3a9b
Compare
export type SearchaliciousHistoryInterface = { | ||
query: string; | ||
name: string; | ||
_currentPage?: number; | ||
_facetsNodes: () => SearchaliciousFacets[]; | ||
_facetsFilters: () => string; | ||
convertHistoryParamsToValues: (params: URLSearchParams) => HistoryOutput; | ||
setValuesFromHistory: (values: HistoryOutput) => void; | ||
buildHistoryParams: (params: BuildParamsOutput) => HistoryParams; | ||
setParamFromUrl: () => {launchSearch: boolean; values: HistoryOutput}; | ||
}; | ||
|
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 will add myself a commit to put it in another file, because I find it strange to have it defined here. (but I take on me).
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.
Hum, it's more complicated than it seems indeed, so I wont do that now !
Thanks however for separating history code…
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.
Thanks @Kout95, let's merge it !
What
q=
Part of