Skip to content
This repository has been archived by the owner on Feb 3, 2024. It is now read-only.

Pages😳 #71

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

ethanf108
Copy link
Member

Added pages to Quotefault so it doesn't load literally every other quote
Pages are size 20

Copy link
Collaborator

@jabbate19 jabbate19 left a comment

Choose a reason for hiding this comment

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

Lgtm, @mxmeinhold reviewed my pagination PR so if they can also see this that would be great

Copy link
Contributor

@mxmeinhold mxmeinhold left a comment

Choose a reason for hiding this comment

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

whoops, forgot to actually hit review on this.

I agree with the goal of pagination, but I'm a little confused how this looks with regard the the ui, and my concern with #68 also applies here: this leaves no way to ctrl-f through all the quotes

//Prepare the url with the proper query strings
let urlParams = new URLSearchParams(window.location.search);
let speaker = urlParams.get('speaker');
let submitter = urlParams.get('submitter');
let urlStr = `/additional`;
let page = urlParams.get('page');
let urlStr = `/additional?j`;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is j here?

//Prepare the url with the proper query strings
let urlParams = new URLSearchParams(window.location.search);
let speaker = urlParams.get('speaker');
let submitter = urlParams.get('submitter');
let urlStr = `/additional`;
let page = urlParams.get('page');
Copy link
Contributor

Choose a reason for hiding this comment

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

does this ever get used?

Comment on lines -54 to +58
$("#moreQuotes").html(data)
$("#moreQuotes").append(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit unsure of how this looks in practice, could you send some redacted/dev screenshots? otherwise I should be able to throw up a local dev instance at some point soon

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants