Making WordPress.org

Opened 10 years ago

Closed 8 years ago

Last modified 3 years ago

#470 closed enhancement (wontfix)

Add Infinite Scrolling to WordPress Events plugin

Reported by: iandunn's profile iandunn Owned by: iandunn's profile iandunn
Milestone: Priority: normal
Component: Make (Get Involved) / P2 Keywords: needs-patch good-first-bug needs-testing
Cc:

Description

The Official WordPress Events plugin displays WordPress-related events that are pulled in from the APIs on Meetup.com and WordCamp.org.

It currently loads the first 50 events, but we'd like to support infinite scrolling as well.

Attachments (5)

official-wordpress-events.diff (1.4 KB) - added by utkarshd_42 10 years ago.
Diff for official-wordpress-events.php
template-events.diff (697 bytes) - added by utkarshd_42 10 years ago.
Diff for template-events.php
patch_470_meta.diff (3.3 KB) - added by bansod_deven 10 years ago.
Edited Patch for #470 meta
470_patch_2.diff (5.2 KB) - added by utkarshd_42 10 years ago.
The patch with the corrections suggested by Ian, patch 2 for meta #470
470_patch_3.diff (4.9 KB) - added by iandunn 10 years ago.
Refresh of 470_patch_2.diff

Download all attachments as: .zip

Change History (16)

#1 @iandunn
10 years ago

  • Component changed from wordcamp.org to make.wordpress.org / P2

#2 @bansod_deven
10 years ago

  • Cc devenbansod.bits@… added
  • Keywords reporter-feedback added

I went through the MeetUp API request we are making to get the MeetUp Events in the function 'get_meetup_events()' on line 117.

In MeetUp API request, I guess we can remove 'page' argument and we will be done but I am not sure as I could not find any default value for 'page' argument on MeetUp Documentation.

I could not find a specific MAX Value allowed for 'page' either but a forum on drupal.org mentions that max value is 200 and if we put 'page=0' it automatically changes to 'page=200'.

Forum Link : https://www.drupal.org/node/949246

But how do we add an option to change between some constant Results and Infinite Results ?

What I had in mind was we can check if the 'POSTS_PER_PAGE' constant value in 'official_wordpress_events.php' is '0' then it will automatically change it to '200' according to the mentioned Forum Posts.

And regarding the WordCamp events, there is no such upper limit mentioned in this code.

Please suggest changes and I will then add the patch accordingly.

Last edited 10 years ago by bansod_deven (previous) (diff)

#3 @iandunn
10 years ago

  • Keywords reporter-feedback removed

We're not looking to load all of the events up front, we still want to initially load 50 of them, but then we want to fetch more events as needed. Infinite scrolling refers to the design pattern where additional content is lazy-loaded as the user approaches the end of a page.

Here's some more info on it:

@utkarshd_42
10 years ago

Diff for official-wordpress-events.php

@utkarshd_42
10 years ago

Diff for template-events.php

#4 @utkarshd_42
10 years ago

Added the infinite scrolling functionality (Diffs provided above). Works on my WordPress installation. Needs further testing.

The above diffs are not in a proper format please see - https://github.com/utkarshd420/official-wordpress-events, for better readability.

Last edited 10 years ago by utkarshd_42 (previous) (diff)

#5 @bansod_deven
10 years ago

  • Keywords needs-testing dev-feedback added

Hey Utkarsh,

The Patch looks good.

But as you mentioned, the diffs uploaded here do not seem to be in proper format. Also, you don't have to upload 2 different patches for 2 different files. Changes in both files can be included in the same patch. Just go to the root of your working copy. Just do

svn diff > patch_name.diff

If you have added any new files not present in working copy(which is not the case here), then you might have to do

svn add path_to_new_file

Also there were some formatting and coding standards mistakes that I found, I have changed them accordingly and also merged the 2 patches into one. Please review if the changes are right or you can always remove those changes.

Edited Patch Added.

Needs Testing though. Please make any changes if you want.

@bansod_deven
10 years ago

Edited Patch for #470 meta

#6 @iandunn
10 years ago

Thanks Utkarsh and Deven, here are a few things that can be improved:

  • Don't remove braces from single-line statements inside conditions - https://make.wordpress.org/core/handbook/coding-standards/php/#brace-style. In general, it's a bad idea to make changes that are unrelated to the ticket, because they add diff noise and prevent an organized and logical commit history.
  • $off should be validated to make sure it's an integer
  • $off isn't a meaningful variable name; use $offset instead.
  • Instead of wp_die(), use wp_send_json_success() and wp_send_json_error()
  • Instead of returning the HTML output, just return the JSON-encoded data, and then build the elements in JavaScript, based on a template. See https://lkwdwrd.com/wp-template-js-templates-wp/ for a tutorial.
  • Add the parameter type for $offset in the phpDoc
  • Follow the coding standards for variable declaration at the beginning of JavaScript functions.
  • Prefix the infinite_scroll action to avoid conflicts with other plugins
  • Is async_infinite_load() triggered when the user scrolls to within 10px of the bottom? If so, that should be more like 300px, so that there's plenty of time for the HTTP request to return before the user gets to the end of the page
  • The JavaScript should be wrapped in a literal to avoid variables/functions being global.

Also, since most of WordPress.org has a footer, we probably trigger async_infinite_load() when the user clicks on a 'Load More Events' button, rather than automatically. If it's automatic, then the user won't be able to reach the footer until they scroll through an unreasonable number of "pages."

#7 @iandunn
10 years ago

It'll also need an wp_ajax_infinite_scroll action, for logged-in users.

#8 @utkarshd_42
10 years ago

Thanks Ian, for your feedback and suggestions. I'll make the necessary changes and submit a new patch as soon as possible.
Yes, I also think that we should load more events only when the user clicks on a Load more button. I'll remove the scroll() jQuery function and add a button or a href element to load more options.

Last edited 10 years ago by utkarshd_42 (previous) (diff)

#9 @utkarshd_42
10 years ago

Hi,

Following is the patch with the above suggested changes by Ian. I've tried my best to stick to the coding standards but being a newbie I might have made some mistakes. The patch attached is working correctly. Added a load more link too instead of automatic loading. I've used the wp.template as well (was really cool!! Thanks Ian) .
I also increased the date of events that the plugin gets from 1 month to 4 months Mostly because, often there are not more than 50 events in a month

Last edited 10 years ago by utkarshd_42 (previous) (diff)

@utkarshd_42
10 years ago

The patch with the corrections suggested by Ian, patch 2 for meta #470

@iandunn
10 years ago

Refresh of 470_patch_2.diff

#10 @iandunn
10 years ago

  • Keywords dev-feedback removed
  • Owner set to iandunn
  • Status changed from new to accepted

Thanks, this looks good. I think the next step is to get WordCamps pulled in too. The JSON API isn't currently active on wordcamp.org, but if you change WORDCAMP_API_BASE_URL to point to your local sandbox you can simulate it. You can use the Meta Environment to setup a local version of WordCamp.org to pull sample data from.

Here's a few minor things I noticed:

  • wp_json_send_*() functions calls die(), so you don't need to return after them.
  • wp_localize_script() is the standard method for outputting JavaScript variables like ajaxurl
  • ajaxurl should have the plugin prefix, to avoid conflicts with the Core variable that other plugins may enqueue
  • $offset++ is a cleaner way of writing $offset = $offset + 1;.
  • esc_url() is more restrictive than esc_attr(), so it's not necessary to wrap esc_url() calls in esc_attr().
  • Data passed to wp_send_json_*() will be automatically passed through json_encode(), so you don't need to do it manually.

I uploaded a refreshed patch, since some other changes were made to the plugin since it was uploaded.

#11 @samuelsidler
8 years ago

  • Resolution set to wontfix
  • Status changed from accepted to closed

After checking with @iandunn, I'm going to wontfix this in favor of a WordPress.org Events API that's being worked on now. There will be several entry points / front-ends over time.

Note: See TracTickets for help on using tickets.