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

Enhancement: Improve Documentation & Yoda Condition #290

Merged
merged 5 commits into from
Mar 17, 2017

Conversation

ahmadawais
Copy link
Contributor

@ahmadawais ahmadawais commented Mar 17, 2017

This PR improves the documentation as per the WordPress Documentation Standards. It also addresses a Yoda Condition error.

Replaces #278 h/t @aduth and @youknowriad
Looking forward!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This LGTM but I'll leave other reviewers check cause I'm not an expert in WordPress Plugins and WordPress Plugins Documentation :)

@nylen
Copy link
Member

nylen commented Mar 17, 2017

Previously #278. I have similar feedback to what @aduth said over there.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

Adding inline documentation is a good idea, but we need to make sure it adds value rather than just stubs without much content.

index.php Outdated
/**
* Plugin Name: Gutenberg
* Plugin URI: https://wordpress.github.io/gutenberg/
* Description: Prototyping since 1440. Development plugin for the editor focus in core.
* Version: 0.1
*
* @package WordPress
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not add this unless/until this code ships in core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can remove this for now.

index.php Outdated
*/

// Hook.
Copy link
Member

Choose a reason for hiding this comment

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

-1 on this comment and all others that just say "Hook", it doesn't add value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nylen Do you have a suggestion to what should go here? Leaving it without inline docs, feels weird. I appreciate your feedback.

Copy link
Member

Choose a reason for hiding this comment

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

I would leave it without docs, yes. I would also move the calls immediately below the functions where they are defined. This combination of inline docblock, function code, add_action is enough to be very clear, IMO.

WP core only requires documentation for the do_action or apply_filters calls, not the places where the code actually uses a filter (ref).

index.php Outdated
*
* @param string $hook Screen name.
* @since 4.8.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

Also needs a brief explanation.

Enqueues scripts and styles when visiting the top-level page of the Gutenberg editor.

index.php Outdated
* Gutenberg's Menu.
*
* @since 4.8.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

This block should describe (even if just in a short sentence) what the function actually does.

Adds a new menu page for the Gutenberg editor.

index.php Outdated
* Project.
*
* @since 4.8.0
*/
Copy link
Member

Choose a reason for hiding this comment

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

The main entry point for the Gutenberg editor. Renders the editor on the wp-admin page for the plugin.

@ahmadawais
Copy link
Contributor Author

@nylen Thanks for your feedback. I have made the changes, take a look when you are up for it.

@ahmadawais
Copy link
Contributor Author

ahmadawais commented Mar 17, 2017

@nylen Kindly, take a look now.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I made some very small formatting tweaks. This looks good to me, thanks for doing it :)

@nylen nylen merged commit 046be3d into WordPress:master Mar 17, 2017
@ahmadawais
Copy link
Contributor Author

@nylen Thanks for the review. I believe that adding more than 80 characters in a single line of long description is a bad practice which is why I split the long desc at two lines.

Anywho, thanks! I am on it and will try to contribute more.

@nylen
Copy link
Member

nylen commented Mar 17, 2017

I believe that adding more than 80 characters in a single line of long description is a bad practice which is why I split the long desc at two lines.

I agree! I'm not a fan of long lines in code either.

I used my editor to wrap these lines at 80 chars, rather than breaking at the end of a sentence. If you see one that I missed, let me know and we'll fix it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants