Skip to content

Added Query Parameters#382

Open
plantindesk wants to merge 2 commits into
undergroundwires:masterfrom
plantindesk:params
Open

Added Query Parameters#382
plantindesk wants to merge 2 commits into
undergroundwires:masterfrom
plantindesk:params

Conversation

@plantindesk

@plantindesk plantindesk commented Jul 7, 2024

Copy link
Copy Markdown
  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
Copy Markdown
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.

@undergroundwires
undergroundwires force-pushed the master branch 4 times, most recently from 4578a0d to a05a600 Compare September 24, 2024 12:00
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