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

refactor: API #6

Closed
wants to merge 30 commits into from
Closed

refactor: API #6

wants to merge 30 commits into from

Conversation

zameji
Copy link

@zameji zameji commented May 3, 2024

This escalated a bit, main changes:

  1. Updated psycopg from 2 to 3
  2. type hints in most places
  3. Pre-commit for consistent formatting
  4. Restructured code to separate concerns
  5. Added input validation with Pydantic, allowing to drop the ALLOWED_ checks
  6. Restructured query, some primitive optimization (still on my end with a sample query a speedup of factor 1.7x)
  7. Prepared server-side pagination (though some API change would be useful to better support it in frontend, but would take this to separate PR implementing just pagination in front- and backend)

@zameji
Copy link
Author

zameji commented May 8, 2024

This also addresses #3 (via enabled / disabled paging) and implements #4.

@zameji zameji marked this pull request as draft May 8, 2024 22:43
@zameji zameji marked this pull request as ready for review May 8, 2024 23:05
@zameji
Copy link
Author

zameji commented May 27, 2024

Also migrated to Vue3 (as Vue2 is EoL for a while now).
Did also major changes to the UI:

  • better support for horizontal screens - seeing the main map and results at once should work for most resolutions
  • more compact feature selection
  • individual features may be renamed to keep track of them while searching
  • pagination with prefetching the next batch in the background
  • results may be removed from the list (allowing discarding candidates that are clear false positives)
  • better performance on low-end computers even for larger queries
  • dark mode (got this one for free with the upgrade to vue/vuetify3)

Backend changes

There are still some TODOs open:

  • Modularize the store to properly use Pinia
  • The OneTap-Login was dropped for now

@loganwilliams Some further UX improvements seem possible to me, but would require knowing more about how it's used - e.g. do people use the small previews more than the main map?

@loganwilliams
Copy link
Contributor

Thanks for this @zameji, really appreciate your work here. It will take me some time to review it, and I might ask you to break the PR into smaller feature specific chunks. I'll be in touch soon.

Best,
Logan

@zameji
Copy link
Author

zameji commented May 30, 2024

Sure, take your time. Breaking it up is going to be somewhat problematic though, esp. in the frontend, since the migration to Vue3 brought with it a lot of necessary changes.
In the backend, the individual commits should be smaller and easier to review by itself.
If you have any questions/doubts about the design decisions, let me know - I'm open to trying different approaches.

@loganwilliams
Copy link
Contributor

Thanks @zameji, I understand that it might be hard to break up changes. As a first pass, to separate PR concerns, could you make one PR for front-end changes and one PR for back-end changes?

@zameji
Copy link
Author

zameji commented Jun 3, 2024

Sure @loganwilliams I'll look into that tonight / tomorrow night.
The current frontend can't work with the new backend without a small change, though, because the interface changed. I'll include that change in the backend PR.

@zameji
Copy link
Author

zameji commented Jun 3, 2024

Closing this PR as:

@zameji zameji closed this Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants