Revises CONTRIBUTING.md #10054

atsansone · 2024-01-16T23:05:35Z

Revises contributing guidelines. Fixes #10053

flutter-website-bot · 2024-01-16T23:14:28Z

Visit the preview URL for this PR (updated for commit 3f0419c):

https://flutter-docs-prod--pr10054-fix-10053-q885qn7s.web.app

CONTRIBUTING.md

atsansone · 2024-01-23T23:58:18Z

@RedBrogdon , @sfshaza2 , @MaryaBelanger : PTAL.

sfshaza2

Some feedback.

CONTRIBUTING.md

sfshaza2 · 2024-01-24T17:43:01Z

CONTRIBUTING.md

+  rather than commenting on every instance.
+* Try to **approve** with suggestions wherever possible.
+  Use **request changes** only when the change can harm an end user's work or
+  violates [Flutter's Code of Conduct][code-of-conduct].


I would expect that if a PR violates the code of conduct, that we would just close it and close comments on it.

sfshaza2 · 2024-01-24T17:43:36Z

CONTRIBUTING.md

+  Use **request changes** only when the change can harm an end user's work or
+  violates [Flutter's Code of Conduct][code-of-conduct].
+
+### Review the technical concerns


Review the technical concerns? I think it's Review the technical content

sfshaza2 · 2024-01-24T17:45:31Z

CONTRIBUTING.md

+      before merging.
+      Click **Approve** and change the label from
+      [**review.tech**][review-tech-label] to
+      [**review.copy**][review-copy-label].


Again, not all external contributors can apply labels.

Anyone with triage access could, which I believe all reviewers can do. I'm double-checking with @godofredoc .

sfshaza2 · 2024-01-24T17:45:51Z

CONTRIBUTING.md

+      note the reason why,
+      click **Comment** and change the label from
+      [**review.tech**][review-tech-label] to
+      [**review.await-update**][review-await-label].


Understood. Checking this.

MaryaBelanger

Some initial comments. Also, not sure how it renders, but each numbered item in the lists is just a 1.

MaryaBelanger · 2024-01-24T17:57:14Z

CONTRIBUTING.md

+[search for an issue][search-issue] that covers your concern.
+If you don't find one and your change is significant,
+consider [creating an issue][create-issue]
+that describers the problem and how you would plan to fix it.


Suggested change

that describers the problem and how you would plan to fix it.

that describes the problem and how you plan to fix it.

CONTRIBUTING.md

MaryaBelanger · 2024-01-24T18:08:02Z

CONTRIBUTING.md

+
+Before submitting a PR,
+[search for an issue][search-issue] that covers your concern.
+If you don't find one and your change is significant,


If they DO find one, what then? Do they have to announce they're working on it? Wait for approval? Just start working on it? Etc.

We typically encourage users to upvote by adding a 👍 rather than leave "this is affecting me too" comments.

MaryaBelanger · 2024-01-24T18:09:34Z

CONTRIBUTING.md

+
+1. [Create a branch][create-branch] for your change.
+   Consider naming it `fix-<issue_number>` to help others find the related issue.
+   The new branch should be based on the `main` or a feature or version branch.


Suggested change

The new branch should be based on the `main` or a feature or version branch.

The new branch should be based on `main` or a feature or version branch.

MaryaBelanger · 2024-01-24T18:14:35Z

CONTRIBUTING.md

+
+   1. Avoid adjusting indents, line breaks, formatting, and the like
+      unrelated to your PR as part of the PR. Separate different work
+      using a new commit, a separate issue, a separate PR, or some


Would "a new commit" still be in the same PR? The previous sentence says "as part of the PR"

MaryaBelanger · 2024-01-24T18:20:32Z

CONTRIBUTING.md

+### Respond to reviewer feedback
+
+1. Read and address comments from the reviewer.
+   Acknowledge all comments with at least add a [reaction emoji][reaction-emoji].


Suggested change

Acknowledge all comments with at least add a [reaction emoji][reaction-emoji].

Acknowledge all comments with at least a [reaction emoji][reaction-emoji].

MaryaBelanger · 2024-01-24T18:21:34Z

CONTRIBUTING.md

+
+### General guidelines for reviews
+
+* Technical review comes before content review, or "copy", review.


Suggested change

* Technical review comes before content review, or "copy", review.

* Technical review comes before content, or "copy", review.

MaryaBelanger · 2024-01-24T18:24:36Z

CONTRIBUTING.md

+  If the writer makes a change that's technically correct but could be done
+  differently, _approve the PR_.


"technically correct but could be done differently" -- not sure what the idea behind this statement is. Like, you could do something technically correctly in a long, convoluted, messy way. Best practices should be enforced if the reviewer has that context

MaryaBelanger · 2024-01-24T18:25:05Z

CONTRIBUTING.md

+* Critique the _technical aspects_ of the PR.
+  If the writer makes a change that's technically correct but could be done
+  differently, _approve the PR_.
+* Provide suggested changes with corrected copy to make your change clear.


Suggested change

* Provide suggested changes with corrected copy to make your change clear.

* Provide suggested changes with corrected copy to make your changes clear.

MaryaBelanger · 2024-01-24T18:35:50Z

CONTRIBUTING.md

+      [**review.tech**][review-tech-label] to
+      [**review.await-update**][review-await-label].
+
+### Review the content quality


I think these headings should be more clear. It's not clear from the headings or the contents in each section that these are two separate types of reviews, and if you're requested as a reviewer, you're likely only responsible for one (correct?) Something like

Suggested change

### Review the content quality

### Copy review

If you're requested for a _copy_ review... (follow these practices)

That's tricky. I'm trying to keep headings parallel.

sfshaza2

Why does this PR affect close to 60 files? It shouldn't affect much more than 1.

atsansone · 2024-01-24T23:09:43Z

Bad rebase. I'll fix it.

RedBrogdon

LGTM once @MaryaBelanger and @sfshaza2 sign off. I'm really glad to see y'all arriving at the right set of guidelines!

sfshaza2 · 2024-01-31T20:39:46Z

@atsansone, can you address @MaryaBelanger's concerns?

johnpryan · 2024-01-31T21:02:20Z

CONTRIBUTING.md

+  If technical content could be made less complex or more detailed,
+  advise the writer that this is the case in your review.
+  Still _approve the PR_ but only if your changes are included.


Why is this worth calling out? How are technical comments treated differently from other comments?

johnpryan · 2024-01-31T21:03:14Z

CONTRIBUTING.md

+
+To conduct a consistent technical review in the Flutter website:
+
+1. [Review a PR][review-pr] when it meets the following conditions:


This is a lot for me to think about. Shouldn't I just review a PR if I'm asked to review it?

Right now, GitHub actions set who reviews. The hope will be to remove this automatic setting soon. Because of this automation, a review for a PR is requested even if the author is not done with it. The review request via GitHub has lost its meaning as is.

It would be GREAT if we could only add the DRE that is on rotation. The original plan was to use a weekly rotation to reduce the email noise for everyone.

The framework team individually tags reviewers. Maybe we can copy that workflow here for the DRE who's on rotation that week?

CONTRIBUTING.md

johnpryan · 2024-01-31T21:05:34Z

CONTRIBUTING.md

+* Favor trusting the writer.
+  If they stand by the changes and the copy issues are minimal,
+  _approve the PR_.


Does the writer refer to the TW or the author of the PR?

Author. I can clarify that.

johnpryan · 2024-01-31T21:09:02Z

CONTRIBUTING.md

+      [**review.tech**][review-tech-label] to
+      [**review.await-update**][review-await-label].
+
+### Perform a content review


What's the purpose of distinguishing between these two types of reviews? There's a lot of redundant copy in these two sections, maybe we should de-dupe the instructions a bit.

The appropriate reviewer can key in on one section rather than going back and forth across the page.

johnpryan · 2024-01-31T21:10:37Z

CONTRIBUTING.md

+
+[approve-pr](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/approving-a-pull-request-with-required-reviews)
+[add-comments](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-comments-to-a-pull-request)
+[search-issue](https://www.freecodecamp.org/news/github-search-tips/)


We should reference official docs as much as possible: https://docs.github.com/en/issues/tracking-your-work-with-issues/filtering-and-searching-issues-and-pull-requests

johnpryan · 2024-01-31T21:14:18Z

CONTRIBUTING.md

+   1. Explain the reason for the additional changes in the PR summary,
+      if necessary. For guidance on what to include in a PR summary,
+      check out [this section in the Blockly docs][blockly-pr-note]
+      or this [PullRequest.com blog post][good-pr-blog-post].


I don't think the reader is ready to write a PR summary, since they haven't pushed their code yet. You are already giving instructions for this in the "Write the PR summary using the following guidelines" section on line 72. Unless you are talking about the commit message?

johnpryan · 2024-01-31T21:16:02Z

CONTRIBUTING.md

+   Consider naming it `fix-<issue_number>` to help others find the related issue.
+   The new branch should be based on the `main` branch or a feature
+   or version branch.


It doesn't make sense that we would tell the reader what to name their branch. Instead, I would much rather we ask them to use the "fixes #number" syntax as it makes issue management easier: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

This is a suggestion, not a mandate. It gives them direction in case they had no other ideas.

Fixed incorrectly formatted indirect links.

atsansone · 2024-01-31T22:03:57Z

@sfshaza2 , @MaryaBelanger : PTAL.

sfshaza2 · 2024-02-16T03:56:05Z

Hi Tony. Given that last week's meeting resulting a decision to substantively change this PR, I'm going to close it. If you'd like me to work on a new PR, let me know. Otherwise, please open a new PR for the new contributing guide.

atsansone added the st.WIP Issue in progress label Jan 16, 2024

atsansone requested review from parlough, sfshaza2, khanhnwin, domesticmouse and MaryaBelanger as code owners January 16, 2024 23:05

atsansone marked this pull request as draft January 16, 2024 23:05

atsansone force-pushed the fix-10053 branch 7 times, most recently from 22d6153 to f063afb Compare January 16, 2024 23:53

atsansone requested a review from RedBrogdon January 16, 2024 23:53

atsansone added review.copy Awaiting Copy Review and removed st.WIP Issue in progress labels Jan 16, 2024

atsansone marked this pull request as ready for review January 16, 2024 23:54

domesticmouse reviewed Jan 17, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

domesticmouse approved these changes Jan 18, 2024

View reviewed changes

parlough assigned RedBrogdon and sfshaza2 Jan 18, 2024

atsansone assigned MaryaBelanger Jan 19, 2024

sfshaza2 reviewed Jan 24, 2024

View reviewed changes

MaryaBelanger reviewed Jan 24, 2024

View reviewed changes

atsansone requested a review from johnpryan as a code owner January 24, 2024 20:13

sfshaza2 reviewed Jan 24, 2024

View reviewed changes

Tony Sansone added 3 commits January 25, 2024 12:14

Update CONTRIBUTING file

48a6c95

Added line breaks

b7d58d8

Updated per TW feedback

ba9fe97

atsansone force-pushed the fix-10053 branch from 66261e1 to ba9fe97 Compare January 25, 2024 18:14

RedBrogdon approved these changes Jan 25, 2024

View reviewed changes

atsansone unassigned RedBrogdon Jan 27, 2024

atsansone mentioned this pull request Jan 29, 2024

Decide on and author and accessibility guide for the website #1838

Closed

sfshaza2 added review.await-update Awaiting Updates after Edits and removed review.copy Awaiting Copy Review labels Jan 31, 2024

johnpryan reviewed Jan 31, 2024

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

johnpryan reviewed Jan 31, 2024

View reviewed changes

atsansone added 2 commits January 31, 2024 15:59

Fixed links, updated content

dd47035

Fixed incorrectly formatted indirect links.

Fixed internal link

3f0419c

sfshaza2 closed this Feb 16, 2024

	that describers the problem and how you would plan to fix it.
	that describes the problem and how you plan to fix it.

	The new branch should be based on the `main` or a feature or version branch.
	The new branch should be based on `main` or a feature or version branch.

	Acknowledge all comments with at least add a [reaction emoji][reaction-emoji].
	Acknowledge all comments with at least a [reaction emoji][reaction-emoji].


		### General guidelines for reviews

		* Technical review comes before content review, or "copy", review.

	* Technical review comes before content review, or "copy", review.
	* Technical review comes before content, or "copy", review.

		If the writer makes a change that's technically correct but could be done
		differently, _approve the PR_.

	* Provide suggested changes with corrected copy to make your change clear.
	* Provide suggested changes with corrected copy to make your changes clear.

-### Review the content quality
+### Copy review
+If you're requested for a _copy_ review... (follow these practices)


		To conduct a consistent technical review in the Flutter website:

		1. [Review a PR][review-pr] when it meets the following conditions:

Revises CONTRIBUTING.md #10054

Revises CONTRIBUTING.md #10054

Conversation

atsansone commented Jan 16, 2024

flutter-website-bot commented Jan 16, 2024 • edited Loading

atsansone commented Jan 23, 2024

sfshaza2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryaBelanger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpryan Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfshaza2 left a comment

Choose a reason for hiding this comment

atsansone commented Jan 24, 2024

RedBrogdon left a comment

Choose a reason for hiding this comment

sfshaza2 commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsansone commented Jan 31, 2024

sfshaza2 commented Feb 16, 2024

flutter-website-bot commented Jan 16, 2024 •

edited

Loading

johnpryan Jan 31, 2024 •

edited

Loading