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

Documentation: updating lib/PHP README.md #53614

Merged
merged 3 commits into from
Aug 14, 2023
Merged

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Aug 14, 2023

What?

The documentation in /lib/README.md, while full of great advice, does not reflect the common reality.

Of particular note are the guidelines relating to naming conventions and avoiding naming conflicts with Core. They have led to some confusion. See: #51959 (comment)

This PR, by no means comprehensive, rejigs things a bit.

Why?

It's hard to be prescriptive when it comes to naming conventions in Gutenberg. Rather, we can emphasize best practice guidelines and 🤞🏻

Testing Instructions

Do the changes make sense? Are they helpful?
Any typos?

@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/README.md

@ramonjd ramonjd self-assigned this Aug 14, 2023
@ramonjd ramonjd added the [Type] Developer Documentation Documentation for developers label Aug 14, 2023
@ramonjd ramonjd marked this pull request as ready for review August 14, 2023 03:50
@github-actions
Copy link

Flaky tests detected in bddfc4b.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5851738762
📝 Reported issues:

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is great @ramonjd, I love the more descriptive introductions and expanded language — it feels like it's got a more welcoming and guided tone for newer contributors in this update 👍

Since there's quite a lot of changes I found it easier to read and check for flow by looking at the direct file: https://github.com/WordPress/gutenberg/blob/bddfc4b4a0952cb68f7552e87477b2e0643540fa/lib/README.md

This reads very nicely to me. I just left a comment about one sentence, but feel free to disregard, since it's likely subjective!

It might be worth getting another pair of 👀 to double-check the approach in case others have feedback, but this looks like a good approach to me! ✨

lib/README.md Outdated
Comment on lines 61 to 62
The caveat is that no functions or classes with the same names already exist in Core. A quick codebase search will also help you know if your new names are unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

The wording here feels slightly awkward to me. Rather than caveat, can we be explicit that there must not be a duplicate? Perhaps something like:

When doing so, care must be taken to ensure that no duplicate declarations to create functions or classes exist between Gutenberg and WordPress core code. A quick codebase search will also help you know if your new names are unique.

I think that should bridge the sentence above and the sentence below? Feel free to disregard this comment, though!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for proof reading this. I have shamelessly plagiarized your example sentence. Much better. 🙇🏻

Copy link
Member

@oandregal oandregal left a comment

Choose a reason for hiding this comment

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

This is more close to reality than before. Thank you!

@ramonjd ramonjd merged commit ef38a19 into trunk Aug 14, 2023
50 checks passed
@ramonjd ramonjd deleted the update/lib-php-readme branch August 14, 2023 22:56
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 14, 2023
@noisysocks
Copy link
Member

Nice!

@gziolo
Copy link
Member

gziolo commented Aug 15, 2023

Thank you so much for improving the documentation that greatly reflects how the process has evolved 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants