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

Enabling progressive loading of index page content #3485

Merged
merged 15 commits into from Aug 21, 2019
Merged

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Aug 1, 2019

Closes #3443

This PR adds in a "load more results" button to the IndexPage template, with code hooked up in main.js to make that an active button that fetches paged entry sets from the index page for progressively loading the backlog of entries.

test

details

the index page for blog has been set to a "page size" of 4 in the CMS (this is a per-index-page setting that defaults to 12 results per page if left untouched). Currently possible page sizes are 4, 8, 12 (which is probably best), and 24

Screen Shot 2019-08-07 at 12 21 25 PM

@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3485 August 1, 2019 23:13 Inactive
@Pomax Pomax requested a deployment to foundation-mofostaging-pr-3485 August 2, 2019 00:22 Abandoned
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3485 August 2, 2019 00:22 Inactive
Copy link
Collaborator

@mmmavis mmmavis left a comment

Choose a reason for hiding this comment

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

Left a few comments.

Copy link
Contributor

@youriwims youriwims left a comment

Choose a reason for hiding this comment

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

Open questions: if we go with this approach, do we want to make the page_size value a field on the IndexPage, so that we can control the batch size through the CMS? It feels like we probably do, but always good to hear opinions.

🗣 Yes—I fully support this! But in reference to what Mavis said 👉 #3485 (comment), we should give them predetermined batch sizes in the CMS to choose from (e.g. at desktop being able to choose between 3, 6, and 9) instead of letting the editor choose a number. If that's what you were already thinking, then ditto!

@Pomax Pomax changed the title [WIP] enabling progressive loading of index page content (route-based) Enabling progressive loading of index page content Aug 7, 2019
@Pomax Pomax marked this pull request as ready for review August 7, 2019 19:07
@patjouk patjouk temporarily deployed to foundation-mofostaging-pr-3485 August 7, 2019 19:14 Inactive
@Pomax Pomax requested review from youriwims and mmmavis August 7, 2019 19:21
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 8, 2019 15:31 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 8, 2019 20:39 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 9, 2019 16:57 Inactive
@alanmoo alanmoo temporarily deployed to foundation-mofostaging-pr-3485 August 9, 2019 18:33 Inactive
@kristinashu
Copy link

Load more is working great except for when there are several posts. In this PR the text says 4 results for iot because 4 have loaded but there are actually 5 (and it continues to say 4 even after the 5th one has loaded). Could you please update it to say the total number of posts (example: 5 results for iot even when only 4 have loaded)?

filtered page: https://foundation-mofostaging-pr-3485.herokuapp.com/en/blog/tags/iot (5 total)

Screen cap from PR:
image

Copy link
Contributor

@youriwims youriwims left a comment

Choose a reason for hiding this comment

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

Should tags with a space show a hyphen in the pill capsule [not sure what to call this]? I also left a comment/question about the counter getting updated. Otherwise, this is looking great @Pomax 😺
Screen Shot 2019-08-20 at 4 48 37 PM

sidenote: This can be done as follow-up, but is there any way that when load more results is clicked, the entries' text & image loads at the same time or is that a weird ask? I'm just curious of what could be causing the image load delay...or maybe there's just a delay on my end. Let me know if you see it too.

source/js/main.js Show resolved Hide resolved
@Pomax
Copy link
Contributor Author

Pomax commented Aug 20, 2019

That's a good question. @kristinashu I assume it's desirable to have the "filter" tag use the same casing and spacing as in the result card?

@Pomax
Copy link
Contributor Author

Pomax commented Aug 20, 2019

@youriwims the delay is caused by the fact that pressing the button starts the API call, so we'd have to pre-call the next set if we want "instant" results. If that's something we want to do, we should probably do that in follow-up.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 21:28 Inactive
@youriwims
Copy link
Contributor

@youriwims the delay is caused by the fact that pressing the button starts the API call, so we'd have to pre-call the next set if we want "instant" results. If that's something we want to do, we should probably do that in follow-up.

Ah okay, makes complete sense.

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 21:41 Inactive
@kristinashu
Copy link

I assume it's desirable to have the "filter" tag use the same casing and spacing as in the result card?

Yes, that would be great. Thank you for pointig that out @youriwims!

@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 22:25 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 22:31 Inactive
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 22:38 Inactive
@Pomax
Copy link
Contributor Author

Pomax commented Aug 20, 2019

@youriwims @kristinashu PR updated with fixes for the flagged issues

@Pomax Pomax requested review from youriwims and removed request for mmmavis August 20, 2019 22:53
@Pomax Pomax temporarily deployed to foundation-mofostaging-pr-3485 August 20, 2019 23:00 Inactive
Copy link

@kristinashu kristinashu left a comment

Choose a reason for hiding this comment

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

Sweet!

Copy link
Contributor

@youriwims youriwims left a comment

Choose a reason for hiding this comment

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

🌟 🌟 🌟

Totally not a blocker: Something feels odd ux-wise about having a Load More, but the total count of entries available prior to all of the entries being visible. I think it's because when I've usually seen this type of feature it's displayed as 'Page x of y' and then includes a total count of entries...just a random thought 🤔 💭

@Pomax
Copy link
Contributor Author

Pomax commented Aug 21, 2019

@youriwims after talking with @kristinashu about what the label should convey (I had the same X of Y idea), we decided to keep the label as simple as possible and just have it say how many results there are total, rather than how many you're looking at right now and then updating that as you click the "load more" button. This might of course be revisited once we have tons of posts, but at our present volume (and a default page set of 12, rather than the custom 4 for the review app) we're unlikely to run into things feeling weird for a while to come =)

@kristinashu
Copy link

@youriwims I don't think it is that unusual to do it this way. The functionality you mentioned might be more common for paginated pages (example: page 3 of 12) but not necessarily as common for the "view more" type of loading where content load below. And sites that use lazy load (example: Medium) don't even show a total number at all. Here are the first few sites that I like which use the "load more" style and only show the total number of results:

https://thewirecutter.com/search/?s=canada
https://www.nytimes.com/search?query=canada
https://www.thisamericanlife.org/archive?keyword=canada

@Pomax Pomax merged commit bb00c55 into master Aug 21, 2019
@Pomax Pomax deleted the using-route-instead branch August 21, 2019 15:45
@youriwims
Copy link
Contributor

Thanks for explaining @Pomax & @kristinashu (and for the examples!) 😄

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.

Add "load more" JS functionality to the listing page
6 participants