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

Convert filters and search text into query params #5248

Draft
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

Suven-p
Copy link
Contributor

@Suven-p Suven-p commented Oct 26, 2024

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
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]:

  • I have read and understand the pull request rules.

Description

Fixes #3672

Type of change

Please delete any options that are not relevant.

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
  • My changes generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

@Suven-p Suven-p marked this pull request as ready for review October 26, 2024 05:35
@Suven-p
Copy link
Contributor Author

Suven-p commented Oct 26, 2024

The current behaviour with incorrect query params is:

Single invalid status like http://localhost:3000/dashboard?status=40
image

Multiple invalid status like http://localhost:3000/dashboard?status=40&status=23
image

Single invalid "active" filter like http://localhost:3000/dashboard?active=null applies filter for Paused
image

Multiple invalid "active" filters will apply filter for Paused but shows "Active" in selected filter instead of "Paused"
image

The behaviour is same for any number of invalid "tag" filters
image

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Nice work.

One thing that I noticed during testing is that if you "navigate away", the query is removed:

image

=> could you change the links to the parts of the UI that deal with editing,cloning, adding monitors, .. which keep you on the same page visually?

About the error handling:
I think this currently looks a bit weird as noted in your last comment.
Could you add a bit of error handing around this?
Imo just ignoring errors and logging them to the console log is likely fine for this, but having a toast prop up might be better..

src/components/MonitorList.vue Outdated Show resolved Hide resolved
src/components/MonitorList.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm added the pr:please address review comments this PR needs a bit more work to be mergable label Oct 27, 2024
@Suven-p
Copy link
Contributor Author

Suven-p commented Oct 27, 2024

=> could you change the links to the parts of the UI that deal with editing,cloning, adding monitors, .. which keep you on the same page visually?

Done. But I really don't like how this is implemented. Let me know if you have any suggestions.

@Suven-p

This comment was marked as outdated.

@Suven-p
Copy link
Contributor Author

Suven-p commented Oct 30, 2024

@CommanderStorm I have addressed all requested changes.

@Suven-p
Copy link
Contributor Author

Suven-p commented Nov 1, 2024

@CommanderStorm Would you add hacktoberfest-accepted label to this PR?

@Ionys320
Copy link
Contributor

Ionys320 commented Nov 14, 2024

Done. But I really don't like how this is implemented. Let me know if you have any suggestions.

Using router hooks, it can be easilly managed! Here is a link to a Stackoverflow answer about this: https://stackoverflow.com/a/47195471/7826809. Also a link to Vue doc (the Stackoverflow answer seems to be for Vue2, ig it would also works for Vue 3, but anyway): https://router.vuejs.org/guide/advanced/navigation-guards.html. Don't hesitate if you need help!

@Suven-p
Copy link
Contributor Author

Suven-p commented Nov 15, 2024

Using router hooks, it can be easilly managed!

That is the current approach. I meant I couldn't figure out without using global router hook. Had it worked, using in-component guards would be clearer since all updates would be handled by same component. With current approach, there are two places that update the same filter query params.

@Ionys320
Copy link
Contributor

Ionys320 commented Nov 15, 2024

I'm facing an issue: I can't select more than one filter. Here is vieo recordings:
Prod version (working):

386790589-78bad66a-4ecc-437c-91f0-7067084c109c.mp4

This PR (not working) (I'm clicking on the different items I hover), started using docker run --rm -it -p 3000:3000 -p 3001:3001 --pull always -e 'UPTIME_KUMA_GH_REPO=Suven-p:feat-3672-use_query_params_for_filter' louislam/uptime-kuma:pr-test2:

386790609-85f93335-6090-4591-93b9-41ed3715b60a.mp4

@Suven-p
Copy link
Contributor Author

Suven-p commented Nov 16, 2024

The latest commit should have fixed that.

@Ionys320
Copy link
Contributor

Confirmed, it's working as the prod version!

@Suven-p
Copy link
Contributor Author

Suven-p commented Nov 18, 2024

Found another issue. Modifying filter parameters from add monitor page with partially formed fields causes the form to be reset.

@Suven-p Suven-p marked this pull request as draft November 18, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support URL parameters so we can link directly to filtered dashboard view
3 participants