-
Notifications
You must be signed in to change notification settings - Fork 199
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
Solves #1029 - Adds paging to recent packages and recent revisions #1055
Conversation
datafiles/static/browse.js
Outdated
@@ -303,7 +303,8 @@ const createPaginator = () => { | |||
pag.appendChild(createPageLink(maxPage)); | |||
} | |||
const isNowOnLastPage = state.page === maxPage; | |||
pag.appendChild(createPrevNext(state.page + 1, isNowOnLastPage, "Next")); | |||
|
|||
if(!isNowOnLastPage) pag.appendChild(createPrevNext(state.page + 1, "Next")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is value in showing a disabled button, since it will result in a more consistent layout when viewing different pages. What is the motivation for changing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted browse.js to match the backend rendering of the paginator used on the recent page. On the backend it was less cumbersome to remove Next or Prev from the HTML conditionally then it was to toggle a disabled attribute since in the XHTML library there was no function for adding no attributes.
I wanted to do something like this: a ! [href "URL", if hasNext then NoAttr else emptyAttr "disabled"] << "Next"
That best way I could do it was to either to add a empty class or do disabled=True/False. Which browser defaults consider any presence of the disabled attribute whether True or False as disabled.
I can easily return the behavior though. 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added back in showing the disabled buttons with the most recent commit.
datafiles/static/hackage.css
Outdated
@@ -1085,11 +1084,12 @@ a.deprecated[href]:visited { | |||
background: linear-gradient(to bottom, #585858 0%, #111 100%); | |||
} | |||
|
|||
.paginator span { | |||
padding: 0 1em; | |||
.paginator a[href][disabled] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If disabled buttons are no longer shown, this style sheet is unnecessary.
@@ -120,32 +196,30 @@ recentRevisionsURL :: URL | |||
recentRevisionsURL = "/packages/recent/revisions.html" | |||
|
|||
|
|||
recentFeed :: Users -> URI -> UTCTime -> [PkgInfo] -> RSS | |||
recentFeed users hostURI now pkgs = RSS | |||
recentFeed :: PaginatedConfiguration -> Users -> URI -> UTCTime -> [PkgInfo] -> RSS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is too much work, but there is a standard for paginated feeds. Seems to me like there is no way for the feed reader to show the next page using the current iteration of this PR, is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. Text-RSS doesn't have a way to pass multiple Links at the top level or give those links a "rel" attribute. I'd be more then willing to improve Text-RSS but that won't be quick.
c46f183
to
5d309ec
Compare
@ysangkok I'll of course take a closer look before acting on this, but want to know if from your standpoint you think the issues you raised have been sufficiently addressed? |
The paginator doesn't seem to work the same on the recent packages list, which I think it should. For example, I have one package in my index, and the browse page looks like: Meanwhile, the recent packages listing looks like: I don't think the ellipsis should be there. (I don't really care about the pluralization) |
99b7e97
to
2e0de96
Compare
I've fixed the paginator issue @ysangkok described. 👍 |
A maximum page size like e.g. 200 should be enforced. Otherwise, it is really easy to overload the server. Especially since the parameter is received as a query string parameter, and therefore the overloading could be triggered simply by a link. |
7792d95
to
231a70c
Compare
Implemented page size restriction. @ysangkok Do you think the RecentPackages returning all packages is a performance issue? It does use the same caching as before although I wasn't for sure if I refactored it correctly. I can restrict it to only the past 48 worth or some x amount although that will restrict how many pages are possible. |
I think we should restrict that as well, because it seems malicious if somebody is e.g. requesting page 50 of RecentPackages. If a legitimate user needs to do that, they're probably really trying to achieve something that would be better addressed by a new feature. |
* delete unused GitHub Action; fix CI badge * Fix #1076: separate validators from UI and doctest them (#1077) * Cachix caching for nix-shell GitHub Action (#1081) * Add uploaded_at field in package api (#1080) * package page: Include virtual-modules in module tree (#1085) * Allow hashable-1.4 and text-2.0 (#1089) * Divide sitemap into parts * Add sitemap link for subdirectories * Fix `non-canonical-return` warnings * Bump CI to GHC 9.2.3 and restrict to master branch * Check authorisation (#1111) * Dynamically add css piece * Fix #1105: change order of markdown parsers to allow pipes in lists * Fix #1128, fix #1130 by adding bounds to Cabal-syntax and haddock-library * Bump CI to 9.2.4 and some deps * Force .txt and .text to have UTF-8 MIME charset (#1133) * Upgrade to haddock-library-1.11.0 (#1126) * attempt to speed up GitHub Action for Nix Shell * work with cabal 3.8 * Updated accepted licenses (#1092) * Add dependabot for github workflows * Bump cachix/cachix-action from 10 to 12 * Bump actions/checkout from 2.4.0 to 3.1.0 * Bump cachix/install-nix-action from 17 to 18 * Build with Cabal-3.8 and GHC 9.4 (#1141) * Haskell CI: bump to Ubuntu-22.04, GHC 9.2.5 and 9.4.4 * Allow mtl-2.3 and transformers-0.6 (#1150) * Disable test (#1124) * allow disable tests on client side * add deprecated version warning (#1123) * List maintainers on package page (#1098) * List maintainers on package page * Vendor snowball package (#1116) * Add searchbox metadata (#1115) * Add captcha for user registration (#1099) * remove filtering 00-index for cabal version < 2.0 hack (#1152) * Add lastVersion in listings (#749) (#1140) * rm icu dep instructions, add libgd * Add test log display (#1100) * Add test log * Reverse Dependencies indexed on PackageName (#1082) * Rebased Reverse Dependencies * Add "Quick Jump" to candidate package page (#1122) * Solves #1029 - Adds paging to recent packages and recent revisions (#1055) * support for `prefers-color-scheme` (#1008) * 2x brightness for captions and links in dark color scheme * table dark color scheme * prefers-color-scheme for links, footer, and table-of-contents * paginator css for `prefers-color-scheme` * Maintainer notifications * cleanup partial functions for revdeps, elim use of MonadThrow, MonadCatch * fix tests enablement link Co-authored-by: Peter Becich <[email protected]> Co-authored-by: Andreas Abel <[email protected]> Co-authored-by: Hécate Moonlight <[email protected]> Co-authored-by: Matthew Pickering <[email protected]> Co-authored-by: ˌbodʲɪˈɡrʲim <[email protected]> Co-authored-by: Alias Qli <[email protected]> Co-authored-by: Ondřej Kubánek <[email protected]> Co-authored-by: Gautier DI FOLCO <[email protected]> Co-authored-by: Janus Troelsen <[email protected]> Co-authored-by: Levi Butcher <[email protected]>
Implements #1029
Everything Included:
Possible Issues:
Let me know of any improvements I can make. 👍