Skip to content

Feature/simpler view links #1604

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

Merged
merged 3 commits into from
Jul 2, 2025
Merged

Feature/simpler view links #1604

merged 3 commits into from
Jul 2, 2025

Conversation

kwahlin
Copy link
Contributor

@kwahlin kwahlin commented Jul 1, 2025

Enables libris/lxlviewer#1338 (see Jesper's TODO comment). The _limit parameter won't be included in find links for facets, pills, suggestions etc. This isn't a problem since we have a hard coded default limit (20) as fallback when _limit is missing. We probably do want to include _sort however, since e.g. removing/adding a filter shouldn't change the sort order.

kwahlin added 3 commits July 1, 2025 14:09
- Explicitly declare which parameters to include when constructing a find URL.
- Don't include pagination parameters in view links, rely instead on default values (offset=0, limit=20).
- Reduce the number of makeFindUrl methods to avoid confusion.
}
}
case ApiParams.LIMIT -> params.put(ApiParams.LIMIT, "" + limit);
case ApiParams.LENS -> {} // TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have discovered a bug related to the lens parameter. Will investigate it further. No harm in omitting it for now.

Copy link
Member

@andersju andersju left a comment

Choose a reason for hiding this comment

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

Very much an okulärbesiktning but LGTM!

@kwahlin kwahlin merged commit 82d7c2a into develop Jul 2, 2025
1 check passed
@kwahlin kwahlin deleted the feature/simpler-view-links branch July 2, 2025 06:37
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