-
Couldn't load subscription status.
- Fork 207
Fix the code examples #92
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
Conversation
|
@ntwb should I replace all the tabs in the code examples with spaces as well? The code examples on the handbook, when copied will always paste as spaces in the editors, and I'm not sure what's the preferred option when writing markdown code (using tabs or spaces). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dingo-d for this follow-up!
Just left a few small remarks.
Note: I haven't gone through the full text document again as I trust you have (and we all did in the previous thread).
| ); | ||
| ?> | ||
| </div> | ||
| <?php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may need a check that the tabs in code examples in markdown are handled correctly when the page gets converted to the handbook.
In my experience spaces are the way to go with markdown, but this is kind of a special set up, so I'm not sure.
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
|
@GaryJones @ntwb can you give a quick look at this so that I can move on with #93? Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, albeit with one query.
wordpress-coding-standards/php.md
Outdated
| if ( 0 === strpos( 'WordPress', 'foo' ) ) { | ||
| echo __( 'Yay WordPress!' ); | ||
| if ( 0 === strpos( $text, 'WordPress' ) ) { | ||
| echo esc_html__( 'Yay WordPress!' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should one of these examples have a textdomain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're looking at it as a WordPress core code, then no. If we are looking at it as an included theme, then yeah, we should add the textdomain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Not sure what the answer should be as what's described in the handbook are the coding standards for Core primarily (and recommended for the community secondarily).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd leave it without because it's mostly intended to be used for WP core.
Do you agree with that @GaryJones ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the WordPress Coding Standards is to create a baseline for collaboration and review within various aspects of the WordPress open source project and community, from core code to themes to plugins.
The WordPress community developed the standards contained in this section of the handbook, and those standards are part of the best practices that developers and core contributors are recommended to follow.
— https://developer.wordpress.org/coding-standards/wordpress-coding-standards/
I bet there would be far more non-Core developers (i.e. plugin and theme developers) looking at these docs, than WP Core developers or Core contributors, so if we're going to emphasise best practices and expectations, and we have examples like this which already includes textdomains, then it would make sense to be consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're also using esc_html__() here, yet Core would use __() as they don't consider bad/suspicious translations to be a risk, so it's clearly tailored towards non-Core developers; not including the textdomain makes it an example that isn't accurate for either group of developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a textdomain, not a big problem 🙂
Would this be better:
echo esc_html__( 'Yay WordPress!', 'textdomain' );Or should we do something more specific for the textdomain name?
|
@jrfnl when you'll have the time can you check if the addition is ok? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the open question about tabs vs spaces in the code samples, all looks good to me.
This PR contains the suggestions and fixes for the code examples based on comments from #65 PR.
Mostly fixed the escaping issues, some comments missing full stops at the end, and the like.