3 min read
What a maintainer checks in your first PR
The checks a pull request goes through on drift-ui, shown on a real one: the issue link, the scope, the tests and the known gaps.
TL;DR
- Link an issue, stay inside its scope, and explain what the diff can’t show.
- Give numbers for any new dependency, test keyboard use, and make sure CI passes.
- List what you didn’t do. You won’t lose points for naming a gap.
On this page
A good first pull request answers a reviewer’s questions before they ask. To show what that means, we’ll use a real one: pull request #38, which added the phone input.
It closes an issue
The description of #38 opens with “Closes #10”. We read the issue first, because issue #10 says what done looks like: a component, a preview, tests, keyboard and screen reader behavior checked, and updated agent docs. Then we hold your diff up against that list.
Writing Closes #10 also closes the issue when the pull request merges, so nobody has to do it by hand.
The description says what and why
We can read the diff. The description should say what the diff can’t. #38 shows the API in four lines of code, then explains the one behavior a reviewer would question: the field hides the dial code because the country button already shows it.
It stays in scope
This is where #38 is a warning. It also reworked the docs pages, which issue #10 never asked for. The description flags this with “This goes beyond #10, so please review it separately”, and flagging it was right, but your pull request should cover one issue and nothing else. A second pull request is easier to review and easier to merge.
New dependencies come with a reason
Issue #10 left one question open: use libphonenumber-js or ship a small metadata set? #38 picks the library and gives its reasons with numbers: about 36 KB gzipped for metadata on 245 countries, and a hand-kept set would go stale. That’s the level of detail we want for any new dependency.
Tests cover the keyboard
The rule in CONTRIBUTING.md is to test keyboard interaction, not only clicks. #38 added over 80 tests. One of them types germany into the country picker, presses Enter, and checks that the number the user typed survives the switch.
CI passes
CI runs on every pull request, so a failure here is the first thing we see.
| Step | What it catches |
|---|---|
| Format | Files Prettier would change |
| Typecheck | Type errors, against Radix and Base UI |
| Lint | ESLint rule failures |
| Test | A failing Vitest suite, after a registry build |
| Build | Anything that stops the site from building |
Run the same checks before you push:
bun run format
bun run typecheck
bun run lint
bun run testIt says what you didn’t do
The best part of #38 is a section called Known gaps. It says the keyboard flow was checked but not with a real screen reader, that rejected input is announced to screen readers while sighted users see nothing, and that JavaScript per docs page grew from 480.6 KB to 511.5 KB gzipped.
A gap you name is something we can decide on together. You won’t lose points for writing one down, and only tick the checklist boxes for things you actually ran.
Visual changes have screenshots
#38 attaches three screenshots, covering dark and light mode. For any visual change, add a before and after or a short recording so we can see it without checking out your branch.
If you get stuck
You don’t need all of this finished to ask for help. Open a draft pull request with what you have and ask there, or ask on Discord. If nobody has replied after a few days, say so in Discord too.