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

[No QA] update PR checklist #8184

Merged
merged 2 commits into from
Mar 16, 2022
Merged

[No QA] update PR checklist #8184

merged 2 commits into from
Mar 16, 2022

Conversation

marcochavezf
Copy link
Contributor

@marcochavezf marcochavezf commented Mar 16, 2022

Details

This updates the PR checklist for both Contributors and Reviewers based on the recent changes to this doc.
slack convo

Fixed Issues

N/A

Tests

N/A

QA Steps

N/A

Screenshots

N/A

@marcochavezf marcochavezf self-assigned this Mar 16, 2022
@marcochavezf marcochavezf requested a review from a team as a code owner March 16, 2022 18:47
@marcochavezf marcochavezf changed the title update PR checklist [No QA] update PR checklist Mar 16, 2022
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team March 16, 2022 18:54
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Left some comments / questions

- [ ] I followed proper naming convention for platform-specific files (if applicable)
- [ ] I followed style guidelines (in [`Styling.md`](https://github.com/Expensify/App/blob/main/STYLING.md)) for all style edits I made
- [ ] I followed the JSDocs style guidelines (in [`STYLE.md`](https://github.com/Expensify/App/blob/main/STYLE.md#jsdocs))
- [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. “toggleReport” and not “onIconClick”)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation is off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah this is on purpose because this is a sub-bullet point of I followed proper code patterns

Copy link
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh I see, thanks for pointing that, fixing it

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
@marcochavezf
Copy link
Contributor Author

Left some comments / questions

Thanks Aldo! 😀 Could you add the questions related to the checklist points directly in the gdoc?

@aldo-expensify
Copy link
Contributor

Moved my dumb questions to the doc :), I'll mark them as resolved here.

Meanwhile, should we wait for some feedback before approving? Or should I just approved and if my questions translate in any change, we do a new PR?

@marcochavezf
Copy link
Contributor Author

Moved my dumb questions to the doc :), I'll mark them as resolved here.

There are no dumb questions IMO 😄

Meanwhile, should we wait for some feedback before approving? Or should I just approved and if my questions translate in any change, we do a new PR?

This is an ongoing iterative process, so we'll be updating the PR template when there are new suggestions to the gdoc, meanwhile, I think it's fine to merge this PR and create a new one when the questions are resolved in the doc

@aldo-expensify
Copy link
Contributor

This is an ongoing iterative process, so we'll be updating the PR template when there are new suggestions to the gdoc, meanwhile, I think it's fine to merge this PR and create a new one when the questions are resolved in the doc

Sounds good, thanks!

@aldo-expensify aldo-expensify merged commit 1f4ccf2 into main Mar 16, 2022
@aldo-expensify aldo-expensify deleted the marco-updatePRChecklist branch March 16, 2022 20:10
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @aldo-expensify in version: 1.1.44-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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