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

Docs: Add PostsPerPage XML doc #1732

Merged
merged 1 commit into from Nov 6, 2019
Merged

Docs: Add PostsPerPage XML doc #1732

merged 1 commit into from Nov 6, 2019

Conversation

GaryJones
Copy link
Member

See #1722.

@GaryJones GaryJones self-assigned this Jun 20, 2019
@GaryJones GaryJones added this to In progress in XML Documentation via automation Jun 20, 2019
@GaryJones
Copy link
Member Author

I adapted the wording from the sniff, but adjusted it to remove the reference to high post counts coming from the use of posts_per_page = -1. See #1638 for context around this.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Hi @GaryJones Thanks for creating these docs.

I've left some feedback inline.

Aside from that:

  • The "valid" and "invalid" code samples don't mirror each other.
    What I mean by this is that devs - based on the samples here - could get the impression that using _query_posts() is always a bad thing, while using $query_args['key'] is always a good thing.
    I think having a good/bad example for each way to set the keys would be a good idea.
  • The second code comparison is missing <em> tags for all code samples.

I'm also wondering whether the code comparison split on the key used is actually needed or that these could just be combined into one which shows both keys.

Or maybe better yet: whether there should be separate code comparisons for each way to set the keys. That would mean three code comparisons:

  1. Array initialization example.
  2. Adding/overloading a key in an array example.
  3. _query_posts() example.

WordPress/Docs/WP/PostsPerPageStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/PostsPerPageStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/PostsPerPageStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/PostsPerPageStandard.xml Show resolved Hide resolved
WordPress/Docs/WP/PostsPerPageStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/PostsPerPageStandard.xml Show resolved Hide resolved
XML Documentation automation moved this from In progress to Review in progress Jul 9, 2019
@jrfnl
Copy link
Member

jrfnl commented Oct 7, 2019

@GaryJones Just wondering if you'll have a chance to finish this off in the near future.
I'd like to release WPCS 2.2.0 soon and it would be great if this PR could be included.

@GaryJones
Copy link
Member Author

I'm working on this today / tomorrow.

@GaryJones
Copy link
Member Author

I'm also wondering whether the code comparison split on the key used is actually needed or that these could just be combined into one which shows both keys.

Or maybe better yet: whether there should be separate code comparisons for each way to set the keys. That would mean three code comparisons:

I've kept them split as is for now, but I'd be OK with merging them into a single valid + invalid comparisons.

@jrfnl
Copy link
Member

jrfnl commented Nov 3, 2019

@GaryJones Looks like something went wrong with your cross-branch merging as this PR now seems to contains all the changes since WCEU.... Could you please try to fix this ? If you need help getting the branch cleaned up, please let me know.

@jrfnl
Copy link
Member

jrfnl commented Nov 4, 2019

@GaryJones I've had a look at just the doc file, presuming that's the only chance in this PR.

Other than my reply about the placement of the info around the default limit, it all looks good to me.

Once the branch is fixed, I'm happy to approve and merge this.

@GaryJones
Copy link
Member Author

Looks like something went wrong with your cross-branch merging as this PR now seems to contains all the changes since WCEU

@jrfnl Rebased into a single commit, and fixed the branch issue - all clean again :-)

XML Documentation automation moved this from Review in progress to Reviewer approved Nov 6, 2019
Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

Thanks @GaryJones ! Merging now.

@jrfnl jrfnl merged commit ed52f7d into develop Nov 6, 2019
XML Documentation automation moved this from Reviewer approved to Done Nov 6, 2019
@jrfnl jrfnl deleted the docs/posts-per-page branch November 6, 2019 18:51
@jrfnl jrfnl added this to the 2.2.0 milestone Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants