olefredrik / FoundationPress

FoundationPress is a WordPress starter theme based on Foundation 6 by Zurb
https://foundationpress.olefredrik.com
MIT License
2.7k stars 867 forks source link

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

Closed pixelbrad closed 5 years ago

pixelbrad commented 5 years ago

This PR Resolves #1371

This PR has the following changes:

conorbarclay commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Thanks for your contribution, @pixelbrad 👍 Good stuff!

pixelbrad commented 5 years ago

😄 Thanks Ole!