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

Added Query Parameters #382

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

plantindesk
Copy link

@plantindesk plantindesk commented Jul 7, 2024

  1. Added q as search paremeter
  2. Added preset for changing the preset to Standard, Strict etc
  3. Added collection that changes to user defined os

I apologize for sending pull request very late

@undergroundwires
Copy link
Owner

I was thinking about this and here's my feedback to get this on production as part of 0.14.0:

Separate all query handling to its own hook

It's better to separate all URL/routing logic to a separate hook. This way we achieve better separation of concern and can test it update it accordingly. This means that instead of updating three .vue components, you will move the logic to a single file.

To design this we can get inspiration from Vue Router. They use a hook called createRouter. So we can also create a hook like useQueryParameters or similar which we can call from App.vue. This we can place in rc/presentation/componentsroot directly (or a better place if you'd suggest one) asUseQueryParameter.ts`.

We then need to move the code you written from three components to this hook. Maybe three private functions like useQuerySearchParameter, useQueryPreset etc. that this useQueryParameters will call.

Then there are two issues:

  1. How to get update about application?
  2. How to update application?

So we should to exactly how the other UI display components are doing. They primarily inject user selection and collection states to handle this.

You can see presentation.md and especially application.md docs to understand how presentation layer reads application state from application layer and reacts based on events in it.

Btw there was a linting error so it's good to run npm run lint:eslint to fix these errors.

Hope this was clear, please ask if I was not clear on this and you need more information / or you do not agree @plantindesk. This is first time I collaborate someone with someone on UI source code and designing a complex feature, so things may not be well documented/explained.

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.

2 participants