-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add checklist and instructions updating and tophatting documentation changes #1362
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
.github/PULL_REQUEST_TEMPLATE.md
Outdated
* [ ] Tested on [mobile](https://github.com/Shopify/polaris-react/blob/master/documentation/Tophatting.md#cross-browser-testing) | ||
* [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/intro-to-shopify/shopify-admin/supported-browsers) | ||
* [ ] Tested for [accessibility](https://github.com/Shopify/polaris-react/blob/master/documentation/Accessibility%20testing.md) | ||
* [ ] Updated `README.md` with documentation changes |
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 think this might be confusing since there's multiple README.md
files in the repo, can you use language that's bit more specific like if you modified a component, then update that component's README file
or something like that
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.
Updated to specify the component-level README.md
.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
* [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/intro-to-shopify/shopify-admin/supported-browsers) | ||
* [ ] Tested for [accessibility](https://github.com/Shopify/polaris-react/blob/master/documentation/Accessibility%20testing.md) | ||
* [ ] Updated `README.md` with documentation changes | ||
* [ ] Tophatted documentation changes in the style guide |
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 would be nice to include too but there's no easy way of doing this check right now. What if we told people to check in a MD previewer, do you think that's enough? We could also write a quick doc for how to tophat changes in the styleguide and link it here too
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 could also write a quick doc for how to tophat changes in the styleguide and link it here too
Can do. I think testing the look in the style guide is important, since folks in lots of different roles need to do it.
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.
Sounds good, it should just be a matter of documenting the build-consumer
command and telling people to set up polaris-styleguide
as well 👍
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.
Added a new doc, copying over some info from the Tophatting.md
doc and adding instructions.
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.
Looks good to me, I'll let Selene review the content
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 is fantastic. Thanks @dpersing!
I did notice one typo in the new tophatting doc (see below).
Co-Authored-By: dpersing <devon.persing@shopify.com>
Co-Authored-By: dpersing <devon.persing@shopify.com>
…react into doc-check-for-checklist
WHY are these changes introduced?
Currently we don't consistently check to see if documentation needs to change when we make updates to a component.
WHAT is this pull request doing?
Adds PR checklist items for:
README.md
(if necessary)