-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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: Support URL parameters to change Dashboard filters #4496
base: master
Are you sure you want to change the base?
Feat: Support URL parameters to change Dashboard filters #4496
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few requests:
- you currently have a paramerter to enable/disable filters via the query => this is not nessesary
- Please use the vue-router's query attributes instead of
URL
on mount: https://vueschool.io/lessons/router-query-params - It might be better to look at if storing the filters only in the url might be benefitial
- you currently have only the status being filtereable, please include the other filters too
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add URL filtering to the remaining filters - Remove the required `filter=true` parameter - Use vue-router instead of `URL`
Added URL filtering to the remaining filters. Note that tags can be searched by their names and IDs. This means the following - considering the tag
|
@CommanderStorm Not sure I'll be able to do this but I'll give it a try anyway. How do you want this implemented? Everything gets read off the URL queries/parameters or the information gets still stored inside of a variable (filterState) but all the changes get reflected on the URL?
|
The current approach is run via the I would shift the whole state we store for the filters to the URL. uptime-kuma/src/components/MonitorList.vue Lines 98 to 102 in cf2d603
|
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is coming along nicely 👍🏻
I have left a few comments where I think this PR can be simplified
); | ||
|
||
return { | ||
status: filters["status"] ? filters["status"].split(",") : [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you try to store the query as a comma separated string.
I think there is a simpler way, as vue-router seems to allow storing lists ⇒ should be able to retrieve it
https://stackoverflow.com/questions/50692081/vue-js-router-query-array
Is there a reason for this approach I am not seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know that existed. Very nice, will rewrite that part for tags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CommanderStorm Do you want me to rewrite that for every filter? The current approach works well, is it not fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please consider using the tools provided via the vue router instead of inventing a new way of storing lists.
This should significantly clean up the code ^^
Will get this done sometime this week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think, in the current state, another review makes sense.
Please refer to my review above. There are a lot of them which are not resolved
=> let's continue the discussion there.
…/uptime-kuma into dashboard-url-filter-params
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma
Tick the checkbox if you understand [x]:
Description
Support URL parameters to change Dashboard filters.
The following work:
/dashboard?filter=true&status=up
/dashboard?filter=true&status=up&status=down
/dashboard?filter=true&status=up&status=down&status=pending&status=maintenance
If the
filter
URL parameter is set to false, the filters are not going to be in effect.Fixes #3672
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)