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

Add Petitions Dashboard widget #259

Closed
wants to merge 14 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@bahiirwa
Contributor

bahiirwa commented Dec 4, 2018

Issue #40 discussions lead to a feature request of a dashboard widget that shares the petitions suggested by the community.

This PR seeks to add that to core.

How has this been tested?

Initially this was developed as a plugin https://github.com/bahiirwa/ClassicPressPetitionsDashboard and tested with both alpha-1 and beta-1versions before testing in core commits on this branch.

Screenshots

screenshot 2018-12-04 14 04 53

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

bahiirwa added some commits Dec 5, 2018

@bahiirwa

This comment has been minimized.

Contributor

bahiirwa commented Dec 5, 2018

Travis finally green. Transients set up to handle the API calls to 12 hours intervals.

* 'tags [] - array() - The version of the browser the user is using
* 'link - string - The version of the browser the user is using
*/
$response = wp_remote_get( $api_url );

This comment has been minimized.

@dikiyforester

dikiyforester Dec 6, 2018

Contributor

It still needs to be called using AJAX request from the client side.
See how wp_dashboard_primary() works and do the similar way.
Using function wp_dashboard_cached_rss_widget() (or modified version) would be appropriate here.

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

@dikiyforester I think this current approach is OK for a first pass. Using AJAX also has some drawbacks, so it is not a clear trade-off. With AJAX, the initial page load time is slightly faster, but the time to load the content is always slightly longer. This way the content would also depend on JavaScript to appear at all.

This comment has been minimized.

@dikiyforester

dikiyforester Dec 7, 2018

Contributor

@nylen, I think, all ajax drawbacks will never excuse making requests to a remote server on page load. It has a bad hidden effect.

Just keep in mind that this would be an example for other developers who developing similar widgets and if they put few such widgets on the dashboard - the effect would be not so good.

Also, there already is the News widget which uses client-side request. So why it should use a different approach? We actually can reuse utilities it uses.

Anyway, I would add my vision in a parallel branch, just let me know if it worth.

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

Anyway, I would add my vision in a parallel branch, just let me know if it worth.

Maybe a PR against this branch to change over to an AJAX-based implementation? I'm not sure how much of this code you would be able to reuse though.

Semi-related: #259 (comment)

This comment has been minimized.

@dikiyforester

dikiyforester Dec 7, 2018

Contributor

Maybe a PR against this branch to change over to an AJAX-based implementation? I'm not sure how much of this code you would be able to reuse though.

Sure, we'll get something to compare.

if ( ! is_array( $response ) ) {
return;
}

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

How about a more explicit error check here, something like this:

if ( is_wp_error( $response ) || 200 !==  wp_remote_retrieve_response_code( $response ) ) {
    return;
}

This comment has been minimized.

@bahiirwa

bahiirwa Dec 7, 2018

Contributor

Should we add a UX friendly line? Some thing about broken data?

This comment has been minimized.

@nylen

nylen Dec 8, 2018

Member

Sure, good idea. This will come up when the server doesn't have a working internet connection, for example.

We should probably also set a timeout of around 5 seconds (at most) for this request.

if ( is_array( $response ) ) {
$dashboard_petitions_results = $response['body']; // use the content
}

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

This is_array check is redundant.

This comment has been minimized.

@bahiirwa

bahiirwa Dec 7, 2018

Contributor

Really? A little bias to typed languages, What is the returned data is a string? That would leave a mess in widget. I can be corrected.

This comment has been minimized.

@dikiyforester

dikiyforester Dec 7, 2018

Contributor

Really? A little bias to typed languages, What is the returned data is a string? That would leave a mess in widget. I can be corrected.

You're checking that just a couple lines above, and this why the second check is redundant.

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

You're also checking $response here, not the response body.

* @since 1.0.0
* @return array
*/
$json = json_decode( $dashboard_petitions_results, true );

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

This should probably happen before setting the transient, and the result should be checked (an invalid response will cause a JSON parse error, which we should not store in the transient).

There is also a typo Ouput in the comment, but I don't think this comment is necessary at all.

*/
$most_wanted = $json['most-wanted']; // get most wanted (upvoted)
$recent = $json['recent']; // get recent
$trending = $json['trending']; // get trending

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

We can remove these variables...

cp_dashboard_petitions_table_body ( $votesCount, $link, $title, $author, $status, $createdTime );
}
}

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

... and instead of repeating this section of code 3 times, we can use $json[ $list_item ].

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

I would also rename this data array from $json to $petitions. This should fit well with the changes to move json_decode to before the set_transient call.

clickedTab.classList.add('active');
$('#'+ activePaneID).addClass('active');
}
});

This comment has been minimized.

@nylen

nylen Dec 7, 2018

Member

As far as the basic structure of this functionality, there are two ways to go:

  1. Load the petitions data from the browser using an AJAX request to the API endpoint. Make sure an appropriate message is shown with JavaScript disabled.
  2. Load the petitions data on the server using wp_remote_get. Make sure the tabs work with JavaScript disabled.

A benefit of using approach (2) is that the functionality can work using progressive enhancement with JavaScript enabled. So, let's take advantage of this benefit. It can work something like this:

  • The active tab is determined by $_GET['petitions_tab']. The CSS classes for active tab and hidden/visible content are assigned appropriately (and empty( $_GET['petitions_tab'] ) means the first tab is shown).
  • The tabs are structured using <a> elements which point to URLs like ?petitions_tab=TABNAME so that they work without JavaScript enabled.
  • The JavaScript overrides these default URLs to show a new content pane and mark a different tab as active.
@nylen

This comment has been minimized.

Member

nylen commented Dec 7, 2018

Some smaller review points here, which I think can be discussed and addressed in the individual comment threads above.

The biggest thing that's left to do is to make sure the new functionality does something reasonable with JavaScript disabled. Given the way it is structured, it's possible to keep it fully working under this situation.

From https://developer.wordpress.org/themes/advanced-topics/javascript-best-practices/#standard-javascript:

Ensure your site still works without JavaScript first – then add JavaScript to provide additional capabilities. This is a form of Progressive Enhancement.

Regardless of whether WordPress itself has adhered to this guideline in recent versions, it is still a good guideline.

@dikiyforester

This comment has been minimized.

Contributor

dikiyforester commented Dec 7, 2018

From https://developer.wordpress.org/themes/advanced-topics/javascript-best-practices/#standard-javascript:

Ensure your site still works without JavaScript first – then add JavaScript to provide additional capabilities. This is a form of Progressive Enhancement.

Regardless of whether WordPress itself has adhered to this guideline in recent versions, it is still a good guideline.

I think the main idea behind this guideline, is that site itself shouldn't rely on JavaScript. In our case, we have a Petitions widget which is secondary and doesn't affect site functionality anyhow (and shouldn't affect site load as well).

@nylen

This comment has been minimized.

Member

nylen commented Dec 8, 2018

Copy/pasting from above so it doesn't get lost when the PR is updated:

As far as the basic structure of this functionality, there are two ways to go:

  1. Load the petitions data from the browser using an AJAX request to the API endpoint. Make sure an appropriate message is shown with JavaScript disabled.
  2. Load the petitions data on the server using wp_remote_get. Make sure the tabs work with JavaScript disabled.

I am fine with either of these two options as a starting point and I think we should merge whichever one is ready first, then follow up with a separate PR if desired.

$recent = $json['recent']; // get recent
$trending = $json['trending']; // get trending
/**

This comment has been minimized.

@dikiyforester

dikiyforester Dec 9, 2018

Contributor

@bahiirwa Would be good to configure your editor to automatically remove trailing spaces.

@dikiyforester dikiyforester referenced this pull request Dec 9, 2018

Merged

Add Petitions Dashboard widget (AJAX approach) #268

2 of 6 tasks complete
@dikiyforester

This comment has been minimized.

Contributor

dikiyforester commented Dec 9, 2018

@nylen please check out alternative PR #268 made using AJAX approach.

@nylen

This comment has been minimized.

Member

nylen commented Dec 12, 2018

I've just merged #268 (which also includes many of the same commits from this PR). Thanks @bahiirwa and @dikiyforester for your hard work on this feature!

@nylen nylen closed this Dec 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment