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

Adds documentation for WordPress.WP.CronInterval #1823

Merged

Conversation

NielsdeBlaauw
Copy link
Contributor

@NielsdeBlaauw NielsdeBlaauw commented Oct 31, 2019

Related to #1722

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 @NielsdeBlaauw Lekker bezig!

Loved seeing those five PRs coming in.

Reviewed this one and - aside from some nitpicks - there are two points which need attention. Note: these points may well also apply to the other doc PRs you send in:

1. Complying with the coding standards for the examples

Aside from the issue being demonstrated, the code samples should comply with the WP Coding Standards.
That means that closures as callbacks for filter hooks are not allowed (as they can't be unhooked).

While the sniff should still detect these kind of issues, even in code which doesn't comply with the standards, the example code used in the documentation should be "clean".

2. Highlighting of the important bits

The particular bit in the code which is the actual "problem" should be wrapped in <em> tags which will allow the code to be highlighted when the HTML/Markdown generators are used.

WordPress/Docs/WP/CronIntervalStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CronIntervalStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CronIntervalStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CronIntervalStandard.xml Outdated Show resolved Hide resolved
WordPress/Docs/WP/CronIntervalStandard.xml Outdated Show resolved Hide resolved
@NielsdeBlaauw
Copy link
Contributor Author

Ah yes, noticed the <em>'s when viewing another item just now. I'll take the review for this pul requests to all others, so don't bother reviewing those at this time. They'll receive a update tomorrow.

@jrfnl
Copy link
Member

jrfnl commented Nov 1, 2019

@NielsdeBlaauw Excellent! Thanks. The same goes for your earlier PR #1730, so if you have a chance to update that one too.... 😇

@jrfnl
Copy link
Member

jrfnl commented Nov 1, 2019

... so don't bother reviewing those at this time. They'll receive a update tomorrow.

Haven't done a full review of any of them, but I did just have a quick look and left some small remarks here and there which might be helpful for when you are updating the PRs tomorrow anyway.

@jrfnl jrfnl added this to In progress in XML Documentation via automation Nov 1, 2019
Update WordPress/Docs/WP/CronIntervalStandard.xml

Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com>

Update WordPress/Docs/WP/CronIntervalStandard.xml

Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com>

Update WordPress/Docs/WP/CronIntervalStandard.xml

Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com>

Update WordPress/Docs/WP/CronIntervalStandard.xml

Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com>

Update WordPress/Docs/WP/CronIntervalStandard.xml

Co-Authored-By: Juliette <663378+jrfnl@users.noreply.github.com>

Seperates function definition
@NielsdeBlaauw NielsdeBlaauw removed their assignment Nov 1, 2019
XML Documentation automation moved this from In progress to Reviewer approved Nov 4, 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.

LGTM. Thanks for updating it!

@GaryJones GaryJones merged commit 5cc744f into WordPress:develop Nov 4, 2019
XML Documentation automation moved this from Reviewer approved to Done Nov 4, 2019
@jrfnl jrfnl added this to the 2.2.0 milestone Nov 4, 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

3 participants