apache
GH-2267: Display all the pagination options
GitHub issue resolved #2267
Pull request Description:
@LorenzBuehmann first tentative to fix the issue. I was going to change the Pagination Vue component, so that it had an option like maxItems (or max displayed, etc). And set it to some value like 5 or 6 as that would fix in the screen, and show the first 2 or 3, followed by […] , and then the last 2 or 3.
But I know sometimes users may want to know the page they are in, and also be able to jump from page 23 to page 4, which wouldn't be possible doing that.
Not the most beautiful, but maybe this simpler approach to just display everything visibly works OK for this page? Or should we keep it one line only, abbreviating the list? What do you think?
- [ ] Tests are included.
- [ ] Documentation change and updates are provided for the Apache Jena website
- [x] Commits have been squashed to remove intermediate development commit messages.
- [x] Key commit messages start with the issue number (GH-xxxx, or if in JIRA, JENA-xxxx)
By submitting this pull request, I acknowledge that I am making a contribution to the Apache Software Foundation under the terms and conditions of the Contributor's Agreement.
See the Apache Jena "Contributing" guide.
isn't the common approach to to limit the number of pages and simply show a "Next" button or something? And optionaly the last page? I have no experience with Vue, but there is something built-in: https://bootstrap-vue.org/docs/components/pagination
Like this maybe?
Yup, that's another approach, common too. I was just wondering whether we should allow users to select any page number, but I think limiting it to a only a few options is the way to go. Let me change it, show a preview here, and if you think that's looking OK I'll write a test or two and mark as ready for review, @LorenzBuehmann
I have no experience with Vue, but there is something built-in: https://bootstrap-vue.org/docs/components/pagination
Yup, and Bootstrap Vue provides other components that we could use. But I chose to use the plain bootstrap dependency instead, which brings just the CSS/JS from the original Bootstrap code to our list of JS dependencies.
The main reason for that is that in JS once you add a dependency, it normally brings N transitive dependencies (and N may be in the tens, hundreds, thousands :exploding_head: ). Fuseki UI is rather small, so crafting these small components like tables, pagination, forms, is not too hard.
The only complex part is the query editor, but we use an external dependency for that one. Less dependencies, less overhead to maintain security & upgrades.
Thanks!
The current coverage report works well, but the one provided by Vitest UI is like Surefire + Jacoco + a graph with the code, and only using Vitest (i.e. easy to configure). Had to add more dev dependencies, but I hope that's OK.
Current coverage doesn't look bad.
But the Pagination and Table are the only ones lacking now.
Going to add a few ones to make sure we don't have any regressions here, even though the code is easy to debug and fix if anything goes wrong.
And of course writing tests I already found some bugs :bug: :sweat_smile: Going to fix and fix up the commits.
Looks great. Tried "vitest --coverage" as well. 😀
Would it be better to have the graph names sorted so they appear in an predicable, albeit arbitrary, order? It can make it possible to guess how far through a long list to start looking for a particular graph and it'll be roughly the same as graphs are added and deleted, otherwise its "hashset" order.
That can be done in the server - not part of this PR.
Would it be better to have the graph names sorted so they appear in an predicable, albeit arbitrary, order? It can make it possible to guess how far through a long list to start looking for a particular graph and it'll be roughly the same as graphs are added and deleted, otherwise its "hashset" order.
Oh, snap. I looked at that so many times and never thought of that :rofl: big plus 1 for that.
That can be done in the server - not part of this PR.
:+1:
Another future improvement, but now for the UI, would be to keep the page number. I'd have to add the page number to the URL (e.g. /current-url#page=2 or /current-url?page=2), read that param, validate the page exists, and update the component state.
Not too hard, and not a major issue. But nice to have for the future too.
Thanks @afs !
@LorenzBuehmann want to take a final look before it's merged and considered fixed?
One of the recent dependency updates seems to caused git conflict on package.json and yarn.lock.
I think the second choice is correct except that "vitest": "^1.3.1" in this PR.
One of the recent dependency updates seems to caused git conflict on package.json and yarn.lock. I think the second choice is correct except that
"vitest": "^1.3.1"in this PR.
Hadn't noticed the conflict, thanks. I rebased, accepted main's versions, then re-added vitest during rebase interactive, and yarn install. I think it's all done again.
Spoke too soon, accidentally downgraded vite-plugin-istanbul, so had to re-add the latest version from main, and re install, edit commit :face_exhaling: Now it's done! :grimacing:
Going to merge this one, but @LorenzBuehmann feel free to re-open the issue, or create a new one or comment here or on the closed issue if you have any feedback :+1: Cheers
While everything seems to work, I get a non-fatal-to-the-build test failure on a timeout #2331.
It reminds me of what we've had in the past.
It seems to be in a different area.
I wonder if that's the viewport. If you want to test you can try.... hmmm, to increease the browser window during the text. If that doesn't solve it then maybe it's failing because I changed the table component (so just need to update the test :crossed_fingers: ). Thanks Andy!