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.

Sign up for GitHub

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

Jump to bottom

7 send search request to backend #34

Merged
merged 2 commits into from
Jul 11, 2023
Merged

7 send search request to backend #34

merged 2 commits into from
Jul 11, 2023

Conversation

TobiasKampmann
Copy link
Contributor

No description provided.

@TobiasKampmann TobiasKampmann linked an issue Jul 6, 2023 that may be closed by this pull request
Send search request to backend #7
Closed
@netlify
Copy link

netlify bot commented Jul 6, 2023 •
edited
Loading

✅ Deploy Preview for pathoplexus ready!

Name Link
🔨 Latest commit 88db5e0
🔍 Latest deploy log https://app.netlify.com/sites/pathoplexus/deploys/64ad295f418b6f00084ec1c2
😎 Deploy Preview https://deploy-preview-34--pathoplexus.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chaoran-chen
Copy link
Member

chaoran-chen commented Jul 6, 2023 •
edited
Loading

Is this now loading the data on the Astro server side? I.e., we are doing: LAPIS -> Astro server -> browser. Wouldn't it be better to load directly from the client (as is the case for the /sequences page)? (See below)

@chaoran-chen
Copy link
Member

chaoran-chen commented Jul 6, 2023 •
edited
Loading

Strike what I wrote above (and sorry for the noise)! I now understand the implementation better. In principle, I think that it is great that the search is performed on the server side. That's why we are using Astro and what we want from a MPA+SSR-approach. The only weakness of the current implementation that I now see is the use of the MUI data grid. Instead of returning an HTML with the result table, it returns the data embedded in JS and needs React to render it. However, I think that this is not a big issue at this moment and we can rethink/improve it in the future if we want (i.e., it's not a fundamental issue).

@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 0bb063b to 099ec41 Compare July 7, 2023 12:20
@vercel vercel bot temporarily deployed to Preview July 7, 2023 12:20 Inactive
@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 099ec41 to a9bf022 Compare July 7, 2023 14:08
@vercel vercel bot temporarily deployed to Preview July 7, 2023 14:08 Inactive
@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from a9bf022 to 8357ea3 Compare July 7, 2023 14:15
@vercel vercel bot temporarily deployed to Preview July 7, 2023 14:16 Inactive
@loculus-project loculus-project deleted a comment from vercel bot Jul 7, 2023
@chaoran-chen
Copy link
Member

I think that the functionality works great! (The date filters don't work yet but I suggest addressing it in a separate issue: #37)

@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 8357ea3 to 4cb7267 Compare July 10, 2023 08:05
@TobiasKampmann TobiasKampmann mentioned this pull request Jul 10, 2023
Merged
@TobiasKampmann TobiasKampmann marked this pull request as ready for review July 10, 2023 09:11
@chaoran-chen
Copy link
Member

👍 to the functionality! I like how the loading spinner is now in the search button.

@fengelniederhammer, could you do the code review, please? :)

@chaoran-chen
Copy link
Member

Oh, just noticed this:

image

Could you clean up the commit history, please, and remove the Vercel stuff?

@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 4cb7267 to 2ec9638 Compare July 10, 2023 09:27
@TobiasKampmann
Copy link
Contributor Author

Regarding prettier: Can someone check, if the issues with prettier is local, i.e. it is just not running on my machine?

@chaoran-chen
Copy link
Member

I have the same issues with prettier 3 and there are also other reported issues and there is a PR in the prettier-plugin-astro repository: withastro/prettier-plugin-astro#264 (comment). I think it's good to revert to version 2 for now and upgrade again once that PR is merged.

fengelniederhammer
fengelniederhammer requested changes Jul 10, 2023
View reviewed changes
Copy link
Contributor

@fengelniederhammer fengelniederhammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of details, functionality-wise it looks good :)

website/src/components/SearchPage/SearchForm.tsx Outdated Show resolved Hide resolved
website/src/components/SearchPage/SearchForm.tsx Outdated Show resolved Hide resolved
website/src/components/SearchPage/SearchForm.tsx Outdated Show resolved Hide resolved
website/src/components/SearchPage/SearchForm.tsx Outdated Show resolved Hide resolved
website/src/pages/sequences/index.astro Outdated Show resolved Hide resolved
website/src/pages/search/index.astro Outdated Show resolved Hide resolved
website/src/pages/search/search.ts Outdated Show resolved Hide resolved
website/src/components/SearchPage/SearchForm.tsx Outdated Show resolved Hide resolved
@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch 2 times, most recently from 4ce8e65 to 989c224 Compare July 10, 2023 16:14
fengelniederhammer
fengelniederhammer approved these changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@fengelniederhammer fengelniederhammer 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 we also don't need the lapisFakeApi.json.ts anymore. If that's the case, please delete it.

website/src/pages/search/search.ts Show resolved Hide resolved
website/src/pages/search/index.astro Outdated Show resolved Hide resolved
@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 989c224 to 6a80695 Compare July 11, 2023 10:00
@TobiasKampmann
feat(website): send real requests
88db5e0 
* use page redirect instead of react
* simplify config, remove unused code

issue: #7
@TobiasKampmann TobiasKampmann force-pushed the 7-send-search-request-to-backend branch from 6a80695 to 88db5e0 Compare July 11, 2023 10:05
@TobiasKampmann TobiasKampmann merged commit 2e0e9d4 into main Jul 11, 2023
@TobiasKampmann TobiasKampmann deleted the 7-send-search-request-to-backend branch July 11, 2023 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengelniederhammer fengelniederhammer fengelniederhammer approved these changes

@chaoran-chen chaoran-chen Awaiting requested review from chaoran-chen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Send search request to backend
3 participants
@TobiasKampmann @chaoran-chen @fengelniederhammer