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

Set search conditions to query parameters #303

Merged

Conversation

yktakaha4
Copy link
Contributor

This great OSS always helps me.

I use this every day, and I thought it would be more convenient if it was reflected in the query parameters when changing paging or limits.
This makes it easier to share search results with others.

I tested it in my local environment, but I don't know all the features so I may be missing something.
I'm not a front-end expert, so please let me know if you have any concerns.

only paging: /?start=0&limit=100

image

with search condition: /search?q=python+99&start=25&limit=25

image

tag search: /search?q=tag%3A10&start=50&limit=50

image

@axllent
Copy link
Owner

axllent commented May 30, 2024

This is awesome @yktakaha4, thank you! There are a few bugs I have identified which I hope you can maybe fix in your PR:

  1. The idea of a shareable URL is great, except it doesn't work for the start or limit (they just default to 0 and 50 regardless what the URL says). Load Mailpit, go to a page 2, 3 etc (eg: start > 50) and refresh the page (the URL stays the same of course, eg: http://localhost:8025/?start=250&limit=50 but the page actually loads start=0 & limit=50.
  2. Links to tags in either the side nav or the small tags under messages in the overview should always go back to start 0 (they currently use whatever the start currently is). I think that maybe toTagUrl() requires an optional second boolean parameter which sets pagination.start = 0 before the params....
  3. The "inbox" link (that calls reloadInbox()) should include a this.$router.push('/') before this.loadMessages() to "reset" the URL
  4. While viewing any page > 0 - as new messages are received Mailpit, offsets the "current view" in relation to the new messages so that the current view doesn't get affected. You can see the start changing at the top right but the URL remains the same:
    Peek 2024-05-30 16-17
    Ideally the URL needs to change too (dynamically)

You are are able to take a look at those issues for then then great (I suspect the last point I made will be "too hard" so just leave that), but if the rest are too complicated for you (I think you've done a great job so far) then let me know and I'll do some work on it (I'm really busy at the moment, so I'm not sure when I will get to this). Just let me know please so we don't end up both doing the same thing?

@yktakaha4 yktakaha4 force-pushed the feat/search-params-qs branch from e8f9327 to 807c058 Compare May 31, 2024 14:45
@yktakaha4
Copy link
Contributor Author

@axllent Thank you for your kind review and sorry for not checking enough.

I fixed it in the following commit.
807c058

When accessing http://localhost:8025/?start=250&limit=50

image

Initialize start in the tag URL.

image

image

Initialize URL when Inbox is clicked.

image

I also fixed the last issue, but I am not confident.
7e039df

demo.mp4

@axllent
Copy link
Owner

axllent commented Jun 1, 2024

It's working now pretty nicely for me, well done and thank you @yktakaha4! I will merge this into a new feature branch on Mailpit, and I will make a few minor changes (for instance I do not think one needs to convert the start & limit to a string every time).

Thank you very much for all your hard work ❤️

@axllent axllent changed the base branch from develop to feature/query-parameters June 1, 2024 11:31
@axllent axllent merged commit c240293 into axllent:feature/query-parameters Jun 1, 2024
@yktakaha4 yktakaha4 deleted the feat/search-params-qs branch June 1, 2024 12:33
axllent pushed a commit that referenced this pull request Jun 2, 2024
* Set search conditions to query parameters

* Fixed by review

* Update query parameters when new message notified
@axllent
Copy link
Owner

axllent commented Jun 2, 2024

@yktakaha4 I have made a number of small changes to the feature/query-parameters branch which I would really appreciate your feedback (and testing) on - if possible:

  1. The start URL parameter is only set > 0 (shorter URLs)
  2. The limit URL parameter is only set if != 50 (the default)
  3. The URL is now only updated at a maximum of 2x per second when receiving many messages (and having a start > 0) - I was getting a browser error of Too many calls to Location or History APIs within a short timeframe.

The only issue I have found so far (which I don't think can be resolved without a lot of complicated work) is the back browser button doesn't trigger a reload of messages (eg: you click next page, next page, next page, and then click the back browser button). This did not work before your changes either, so it's not new, but it is the only "issue" I can find now. Apart from that, I think this implementation is working very well.

@yktakaha4
Copy link
Contributor Author

I tested it locally and worked fine, thanks.

the back browser button doesn't trigger a reload of messages

As far as I know, we can suppress history generation by using $router.replace instead of $router.push.
Although it is not a fundamental solution, I think it will reduce user confusion caused by the browser's back button.
https://router.vuejs.org/guide/essentials/navigation.html#Replace-current-location

@axllent
Copy link
Owner

axllent commented Jun 2, 2024

Thanks for the suggestion, but I don't think it actually resolves anything or prevents any confusion (in this case). The back browser button still loads a URL which does not change the message listing. Don't worry, it's not an issue unless someone makes it an issue, so I think I'll do a bit more testing and get this branch merged into develop. I just released a new version this weekend so I'll probably wait a few days before releasing another version (OS package maintainers sometimes get grumpy with me when I release too often 😆 ) 👍

Great work, and thank you for your contribution!

@axllent
Copy link
Owner

axllent commented Jun 7, 2024

@yktakaha4 I managed to solve the browser back/forward navigation, shifting around some of the logic plus a few other changes 🥳 I haven't found any other issues in my testing. Thanks again for your hard work, I really appreciate it!

This functionality has now been released in v1.18.5.

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