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

Fix advanced search breadcrumb #4283

Conversation

ThoWagen
Copy link
Contributor

@ThoWagen ThoWagen commented Mar 5, 2025

We realized that the breadcrumb in advanced searches does not include a link back to the search anymore.

Here is a quick fix, but I'm not sure if this is the best way to do it and if there are unwanted side effects. So I created this as a draft and as a starting point for a discussion.

Should we maybe instead call the query parameter for advanced searches sid instead of edit?

@demiankatz
Copy link
Member

@ThoWagen, I don't object to changing the edit parameter of the advanced search screen to use sid instead for consistency. If it doesn't introduce too much complexity, it might be worth adding backward-compatible support for the edit parameter just in case (e.g. if referenced in a local custom template), but I think the likely impact is small.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

One other suggestion: it's probably worth adding a Mink test to cover this. I think we already have a test about editing advanced searches, so it's likely just a matter of adding a new assertion to the existing test. (I can help find it if you're not sure where it is, though hopefully it's not too hard to find).

@ThoWagen
Copy link
Contributor Author

ThoWagen commented Mar 5, 2025

Thanks @demiankatz! I'm not very familiar with the search memory. But since you don't see any concerns here I just switched from 'edit' to 'sid'. Note that 'edit' is still supported and there is a comment including the 'legacy' keyword for now.

I also added a Mink test case.

@ThoWagen ThoWagen marked this pull request as ready for review March 5, 2025 16:46
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @ThoWagen -- see below for a couple more thoughts, but I think this overall approach should work.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @ThoWagen!

@demiankatz demiankatz merged commit d9f4d91 into vufind-org:dev Mar 7, 2025
6 checks passed
@demiankatz demiankatz added this to the 11.0 milestone Mar 7, 2025
ckaz pushed a commit to finc/vufind that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants