feat: show call participants in room list (Discord-style)
Docker / Docker Buildx (push) Has been cancelled
Build Debian package / Build package (release) Has been cancelled
Build and Deploy / prepare (release) Has been cancelled
Deploy release / Deploy to Cloudflare Pages (release) Has been cancelled
Build and Deploy / Trigger Pro pipeline (release) Has been cancelled
Build and Deploy / Windows arm64 (release) Has been cancelled
Build and Deploy / Windows x64 (release) Has been cancelled
Build and Deploy / macOS (release) Has been cancelled
Build and Deploy / Linux amd64 (sqlcipher static) (release) Has been cancelled
Build and Deploy / Linux arm64 (sqlcipher static) (release) Has been cancelled
Build and Deploy / ${{ needs.prepare.outputs.deploy == 'true' && 'Deploy' || 'Deploy (dry-run)' }} (release) Has been cancelled
Build and Deploy / Deploy builds to ESS (release) Has been cancelled
Docker / Docker Buildx (push) Has been cancelled
Build Debian package / Build package (release) Has been cancelled
Build and Deploy / prepare (release) Has been cancelled
Deploy release / Deploy to Cloudflare Pages (release) Has been cancelled
Build and Deploy / Trigger Pro pipeline (release) Has been cancelled
Build and Deploy / Windows arm64 (release) Has been cancelled
Build and Deploy / Windows x64 (release) Has been cancelled
Build and Deploy / macOS (release) Has been cancelled
Build and Deploy / Linux amd64 (sqlcipher static) (release) Has been cancelled
Build and Deploy / Linux arm64 (sqlcipher static) (release) Has been cancelled
Build and Deploy / ${{ needs.prepare.outputs.deploy == 'true' && 'Deploy' || 'Deploy (dry-run)' }} (release) Has been cancelled
Build and Deploy / Deploy builds to ESS (release) Has been cancelled
This commit is contained in:
+120
@@ -0,0 +1,120 @@
|
||||
# Review Guidelines
|
||||
|
||||
The following summarises review guidelines that we follow for pull requests in
|
||||
Element Web and other supporting repos. These are just guidelines (not strict
|
||||
rules) and may be updated over time.
|
||||
|
||||
## Code Review
|
||||
|
||||
When reviewing code, here are some things we look for and also things we avoid:
|
||||
|
||||
### We review for
|
||||
|
||||
- Correctness
|
||||
- Performance
|
||||
- Accessibility
|
||||
- Security
|
||||
- Quality via automated and manual testing
|
||||
- Comments and documentation where needed
|
||||
- Sharing knowledge of different areas among the team
|
||||
- Ensuring it's something we're comfortable maintaining for the long term
|
||||
- Progress indicators and local echo where appropriate with network activity
|
||||
|
||||
### We should avoid
|
||||
|
||||
- Style nits that are already handled by the linter
|
||||
- Dramatically increasing scope
|
||||
|
||||
### Good practices
|
||||
|
||||
- Use empathetic language
|
||||
- See also [Mindful Communication in Code
|
||||
Reviews](https://kickstarter.engineering/a-guide-to-mindful-communication-in-code-reviews-48aab5282e5e)
|
||||
and [How to Do Code Reviews Like a Human](https://mtlynch.io/human-code-reviews-1/)
|
||||
- Authors should prefer smaller commits for easier reviewing and bisection
|
||||
- Reviewers should be explicit about required versus optional changes
|
||||
- Reviews are conversations and the PR author should feel comfortable
|
||||
discussing and pushing back on changes before making them
|
||||
- Reviewers are encouraged to ask for tests where they believe it is reasonable
|
||||
- Core team should lead by example through their tone and language
|
||||
- Take the time to thank and point out good code changes
|
||||
- Using softer language like "please" and "what do you think?" goes a long way
|
||||
towards making others feel like colleagues working towards a common goal
|
||||
|
||||
### Workflow
|
||||
|
||||
- Authors should request review from the element-web team by default (if someone on
|
||||
the team is clearly the expert in an area, a direct review request to them may
|
||||
be more appropriate)
|
||||
- Reviewers should remove the team review request and request review from
|
||||
themselves when starting a review to avoid double review
|
||||
- If there are multiple related PRs authors should reference each of the PRs in
|
||||
the others before requesting review. Reviewers might start reviewing from
|
||||
different places and could miss other required PRs.
|
||||
- Avoid force pushing to a PR after the first round of review
|
||||
- Use the GitHub default of merge commits when landing (avoid alternate options
|
||||
like squash or rebase)
|
||||
- PR author merges after review (assuming they have write access)
|
||||
- Assign issues only when in progress to indicate to others what can be picked
|
||||
up
|
||||
|
||||
## Code Quality
|
||||
|
||||
In the past, we have occasionally written different kinds of tests for
|
||||
Element and the SDKs, but it hasn't been a consistent focus. Going forward, we'd
|
||||
like to change that.
|
||||
|
||||
- For new features, code reviewers will expect some form of automated testing to
|
||||
be included by default
|
||||
- For bug fixes, regression tests are of course great to have, but we don't want
|
||||
to block fixes on this, so we won't require them at this time
|
||||
|
||||
The above policy is not a strict rule, but instead it's meant to be a
|
||||
conversation between the author and reviewer. As an author, try to think about
|
||||
writing a test when making your next change. As a reviewer, try to think about
|
||||
how you might test the area of code you are reviewing. If the reviewer agrees
|
||||
it would be quite difficult to test some new feature, then it's okay for them to
|
||||
accept the change without tests for now, but we'd eventually like to be more
|
||||
strict about this further down the road.
|
||||
|
||||
If you do spot areas that are quite hard to test today, please let us know in
|
||||
[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org). We can
|
||||
work on improving the app architecture and testing helpers so that future tests
|
||||
are easier for everyone to write, but we won't know which parts are difficult
|
||||
unless people shout when stumbling through them.
|
||||
|
||||
We recognise that this testing policy will slow things down a bit, but overall
|
||||
it should encourage better long-term health of the app and give everyone more
|
||||
confidence when making changes as coverage increases over time.
|
||||
|
||||
For changes guarded by a feature flag, we currently lean towards prioritising
|
||||
our ability to evolve quickly using such flags and thus we will not currently
|
||||
require tests to appear at the same time as the initial landing of features
|
||||
guarded by flags, as long as (for new flagged features going forward) the
|
||||
feature author understands that they are effectively deferring part of their
|
||||
work (adding tests) until later and tests are expected to appear before the
|
||||
feature can be enabled by default.
|
||||
|
||||
## Design and Product Review
|
||||
|
||||
We want to ensure that all changes to Element fit with our design and product
|
||||
vision. We often request review from those teams so they can provide their
|
||||
perspective.
|
||||
|
||||
In more detail, our usual process for changes that affect the UI or alter user
|
||||
functionality is:
|
||||
|
||||
- For changes that will go live when merged, always flag Design and Product
|
||||
teams as appropriate
|
||||
- For changes guarded by a feature flag, Design and Product review is not
|
||||
required (though may still be useful) since we can continue tweaking
|
||||
|
||||
As it can be difficult to review design work from looking at just the changed
|
||||
files in a PR, a [preview site](./pr-previews.md) that includes your changes
|
||||
will be added automatically so that anyone who's interested can try them out
|
||||
easily.
|
||||
|
||||
Before starting work on a feature, it's best to ensure your plan aligns well
|
||||
with our vision for Element. Please chat with the team in
|
||||
[#element-dev:matrix.org](https://matrix.to/#/#element-dev:matrix.org) before
|
||||
you start so we can ensure it's something we'd be willing to merge.
|
||||
Reference in New Issue
Block a user