-
Notifications
You must be signed in to change notification settings - Fork 9.4k
magento/magento2#: Fix release notification README #24489
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
magento/magento2#: Fix release notification README #24489
Conversation
Hi @atwixfirster. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
Hi @VladimirZaets, thank you for the review.
|
@@ -1,5 +1,3 @@ | |||
# Magento_ReleaseNotification Module |
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 issue with README was related to an extra space at the beginning of the header. There is no need to remove the entire header, only the extra space.
Change to:
# Magento_ReleaseNotification module
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 issue with README was related to an extra space at the beginning of the header. There is no need to remove the entire header, only the extra space.
done
Thanks, @dshevtsov
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.
@dshevtsov, you're correct about the original issue, but I think there's some inconsistency across module READMEs. Some have the title, but some don't. It makes sense to remove this because we already include the module title in the metadata for the devdocs topic where we use this content. Having duplicate titles in devdocs is not ideal.
I suppose we can solve this in a separate issue since we already have inconsistency in devdocs.
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.
@jeff-matthews, there are several reasons to include the header:
- Our template contains the header.
- READMEs belong to the codebase at first place, so they should have a root header there.
- devdocs site generator processes h1 headers as titles.
- MRG topics are generated using custom tooling, so the automation can be customized to process READMEs differently depending on whether the h1 header is there.
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 need to update the devdocs tooling then. I don't want to see duplicate H1s in the MRG on devdocs anymore.
3524eda
to
e7fb5d7
Compare
Hi @jeff-matthews, thank you for the review.
|
Hi @atwixfirster, thank you for your contribution! |
Description (*)
Provides fix for DevDocs magento/devdocs#5246
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)
CC: @jeff-matthews
Thank you!