Update Review Guidelines #25
Conversation
github-actions
bot
requested review from
eito-fis,
kgodey,
mathemancer,
pavish,
seancolsen and
silentninja
and removed request for
a team
ghislaineguerin
removed request for
eito-fis,
mathemancer,
pavish,
seancolsen and
silentninja
There was a problem hiding this comment.
@ghislaineguerin The updates look good overall, I've requested some changes.
Please also change your PR description to "Fixes #18". This will link the PR to the issue and also ensure that #18 is automatically closed when this PR is merged.
design/process/review-guidelines.md
Outdated
|
|
||
| # Review Process | ||
| The spec creation process begins when a design issue is started and finalizes when the spec is approved and merged. When a design issue is ready for review, a spec is created, and a PR is opened. Finalizing a design spec means, in most cases, that the design issue has also been solved. If this is not the case, the reason and additional requirements must be communicated in the design issue and the spec. |
There was a problem hiding this comment.
Please add something about needing to create additional GitHub issues to track unresolved requirements in case the design issue is not solved.
There was a problem hiding this comment.
It might also be worth explaining what a "design issue" is; new contributors may not be familiar with our process of creating GitHub issues with the work: design label to track design problems.
design/process/review-guidelines.md
Outdated
|
|
||
| ## Authors | ||
| - Write your design spec on the wiki under the [Documents](/design/specs) folder and link to it from the top-level [Documents](/design/specs) page. | ||
| ## Review Process |
There was a problem hiding this comment.
Why make this a second level heading? We don't have any first level headings.
I think it would also be good to explain the review process under the heading here. Maybe move this from the first paragraph:
Every spec goes through a review process so that the team can discuss it and ask questions.
I think it would also be good to explain the goals of the review process here. Why is the team discussing it and asking questions? I can think of some answers: to make sure the design is technically feasible, to ensure that all the scenarios needed for implementation are covered, to look at the design from the perspective of a new or advanced user and imagine using the product, etc. You may be able to think of more reasons. I think having those here will help reviewers.
design/process/review-guidelines.md
Outdated
| - Once you think the spec is ready for review, create a GitHub Discussion that includes: | ||
| - the design document and all related resources such as wireframes, prototypes, etc. | ||
| - Add the [spec content](##spec-content) to the page. Make sure the page date is updated as well. | ||
| - Once you think the spec is ready for review, create a PR that includes: |
There was a problem hiding this comment.
I assume by "includes" you mean in the PR description? Please specify.
design/process/review-guidelines.md
Outdated
| - the design document and all related resources such as wireframes, prototypes, etc. | ||
| - Add the [spec content](##spec-content) to the page. Make sure the page date is updated as well. | ||
| - Once you think the spec is ready for review, create a PR that includes: | ||
| - a link to the referenced design issue from the [Mathesar Repo](https://github.com/centerofci/mathesar) |
There was a problem hiding this comment.
This should be listed as "Fixes <link to the issue>" so that the issue will be automatically closed when the PR is merged.
design/process/review-guidelines.md
Outdated
| - Specify explicitly if you want another look at the design spec before it is finalized. | ||
| - Assign the required reviewers by their GitHub username. | ||
| - Always request a review from the Mathesar members with Product or Design roles (see [Team](/team) for handles. | ||
| - If the issue contains technical implementation details, you can request a review from a member with Engineering role as well. Keep in mind that a review from a single Engineering role member should be enough for most design issues. If you are tagging more than one Engineering member, specify the type of feedback that is required from them. |
There was a problem hiding this comment.
We decided in yesterday's meeting that we would assign one backend engineer and one frontend engineer (with rotation among backend and frontend engineers) for each spec.
design/process/review-guidelines.md
Outdated
| ### Reviewers | ||
|
|
||
| - Check for outstanding [design spec review requests](https://github.com/centerofci/mathesar-wiki/pulls?q=is%3Aopen) at least once a day. | ||
| - Comment on the GitHub PR with feedback. Please follow the general advice above. |
There was a problem hiding this comment.
the advice is actually below, not above :)
design/process/review-guidelines.md
Outdated
| - By sharing feedback, you actively participate in the UX process; make sure you read the context documents and are clear on the user's needs. If needed, ask questions, don't make assumptions. | ||
|
|
||
| # Advice for Reviewers | ||
| #### Advice for Reviewers |
There was a problem hiding this comment.
Please have this be a top level heading so that it's easier to find from the table of contents on the wiki.
design/process/review-guidelines.md
Outdated
| - Use descriptive words such as "clear", "helpful", "obvious", "confusing", "complex", rather than "bad", "wrong", "off", etc. | ||
| - Think backward from the end-goal |
There was a problem hiding this comment.
| - Think backward from the end-goal | |
| - Think backwards from the end-goal |
design/process/review-guidelines.md
Outdated
| - Go from simplest to complex so that new details are added incrementally, for example, 'User opens a table' to 'User opens multiple tables at once. | ||
| - A link to a prototype or wireframes or any artifact used to demonstrate the design solution. | ||
| - Including video walkthroughs for prototypes or wireframes can help the team better understand the proposed solution. |
There was a problem hiding this comment.
maybe mention that we have a Loom account you can request access to if you don't have access already.
|
@ghislaineguerin I made some updates for clarity in 4c2b2e8 |
Fixes #18