Skip to content
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

Merged
merged 16 commits into from
Jun 18, 2024

Conversation

Kout95
Copy link
Collaborator

@Kout95 Kout95 commented Jun 10, 2024

@Kout95 Kout95 linked an issue Jun 10, 2024 that may be closed by this pull request
@Kout95 Kout95 marked this pull request as ready for review June 10, 2024 15:46
@Kout95 Kout95 force-pushed the 126-add-the-possibliity-to-launch-search-on-launch branch from b4eb889 to f43e759 Compare June 10, 2024 15:46
@teolemon teolemon added ✨ enhancement New feature or request query parameters labels Jun 11, 2024
Copy link
Member

@alexgarel alexgarel left a 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

frontend/src/utils/index.ts Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-ctl.ts Outdated Show resolved Hide resolved
frontend/src/search-pages.ts Outdated Show resolved Hide resolved
frontend/src/search-pages.ts Outdated Show resolved Hide resolved
frontend/src/search-pages.ts Outdated Show resolved Hide resolved
@Kout95 Kout95 force-pushed the 126-add-the-possibliity-to-launch-search-on-launch branch from 81a626d to 0a51cb9 Compare June 13, 2024 08:26
@Kout95 Kout95 requested a review from alexgarel June 13, 2024 10:17
@Kout95 Kout95 force-pushed the 126-add-the-possibliity-to-launch-search-on-launch branch from e5c7f28 to 7d053cc Compare June 13, 2024 12:38
Copy link
Member

@alexgarel alexgarel left a 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)

frontend/src/utils/index.ts Outdated Show resolved Hide resolved
frontend/src/search-pages.ts Outdated Show resolved Hide resolved
@Kout95 Kout95 requested a review from alexgarel June 18, 2024 07:13
@Kout95 Kout95 force-pushed the 126-add-the-possibliity-to-launch-search-on-launch branch from bd845b7 to 1d6647c Compare June 18, 2024 07:24
@Kout95 Kout95 force-pushed the 126-add-the-possibliity-to-launch-search-on-launch branch from 1d6647c to d7a3a9b Compare June 18, 2024 07:54
Comment on lines +14 to +25
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};
};

Copy link
Member

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).

Copy link
Member

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…

Copy link
Member

@alexgarel alexgarel left a 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 !

@Kout95 Kout95 merged commit 17fabbb into main Jun 18, 2024
6 checks passed
@Kout95 Kout95 deleted the 126-add-the-possibliity-to-launch-search-on-launch branch June 18, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add the possibliity to launch search on launch
3 participants