Skip to content
This repository has been archived by the owner on Sep 16, 2019. It is now read-only.

Fixes foundationpress_pagination() - Outputs valid HTML, output matches Foundation's docs. #1372

Merged
merged 3 commits into from Apr 11, 2019

Conversation

pixelbrad
Copy link
Contributor

@pixelbrad pixelbrad commented Mar 22, 2019

This PR Resolves #1371

This PR has the following changes:

  • Moves str_replace and preg_replace match patterns to their own arrays.
  • Moves str_replace and preg_replace replacements to their own arrays.
  • Numerous calls to str_replace and preg_replace have been replaced by one call each, with args populated by arrays.
  • Numerous match patterns are more concise, so no HTML tags are accidentally replaced.
  • Numerous match patterns have been updated to match Foundation's documented markup.
  • Replace prev_text and next_text with empty strings, as the new markup automatically inserts left/right arrows.
  • Wrap pagination output in a <nav> element.

@conorbarclay
Copy link
Contributor

conorbarclay commented Mar 26, 2019

While this is absolutely an improvement over the existing implementation, I did a bit of testing with it today and noticed one small error...evaluating $pagination_links right before final output will always return true due to the appending of the <nav> tag on the $pagination_links string...there's a few ways around this (and if I had some time I would drop in a fix), but one approach would be to check $paginate_links earlier on (ideally right after the paginate_links() function call, before all the finding/replacing, appending markup, etc.), or just change name of the final output string (while still evaluating $paginate_links).

Hopefully that made sense!

@pixelbrad
Copy link
Contributor Author

Yep, that absolutely makes sense. Good catch, that was an oversight on my part for sure.

I can find some time this week to implement one of those two options. However, I'm not familiar enough with git/github to know whether or not this pull request will be made aware of another commit to my fork. What course of action do you recommend? What's coming to my mind is, close/cancel this PR, commit the changes to my fork, open a new PR. Does that work?

@pixelbrad
Copy link
Contributor Author

Actually I did a little bit of brainstorming on this. I think it might actually be a better practice to wrap all the modifications to $paginate_links in one if statement (the example is condensed to get the point across):

$paginate_links = paginate_links( ...... );

if ( $paginate_links ) {
    $preg_find = [ ... ];

    $preg_replace = [ ... ];

    // etc. etc.

    echo $paginate_links
}

In my mind, I asked myself the question, if $paginate_links comes back empty, why even bother attempting to run preg_replace and str_replace on it? I'm concerned PHP might throw a warning in the debug log, or in the PHP error log. Since all of these operations require $paginate_links to have some content, why not wrap it all in an if-statement to indicate as such?

But I'm still faced with the dilemma. I can implement this change in my fork no problem, but I'm still unsure what the best workflow is to bring it in to this repo, since this PR is still open. Sorry! Bit of a git newbie still.

@olefredrik olefredrik merged commit 4ffae52 into olefredrik:master Apr 11, 2019
@olefredrik
Copy link
Owner

Thanks for your contribution, @pixelbrad 👍 Good stuff!

@pixelbrad
Copy link
Contributor Author

😄 Thanks Ole!

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.

None yet

3 participants