Reviewing markup and CSS

Reviewing HTML and CSS is different from reviewing other code, and the difference is why UI review is often done badly. There is no logic to trace and no test suite to trust. The interesting questions are about what the markup means, whether the CSS will survive change, and what happens to somebody using the page differently from you.

So a UI review is mostly opening the thing and using it, and only then reading the diff.

Open it before you read it

The single most useful habit, and it inverts what people do.

Every deploy platform gives you a preview URL per pull request — module 6's point. Open it and spend three minutes:

At 320px. Any horizontal scrollbar? Anything unreadable?
At 1440px. Any line of text running the full width?
Tab through it. Everything reachable, visible focus, sensible order?
With the test fixture content. A long name, an empty field, a broken image.
Dark mode and reduced motion, in the Rendering panel.

Three minutes finds things no amount of diff-reading will, because CSS is a system and the diff is a fragment of it. A change to .card that looks fine in isolation may break a card in the sidebar, and the only way to know is to look at the sidebar.

Then read the diff, in this order

1. The HTML. It carries the meaning, and meaning problems are the expensive ones.

Is every element the right one, or is a <div> doing a <button>'s job?
Do the headings form a sensible outline with no skipped levels?
Is there one <h1> and a <main>?
Does every image have an alt that replaces it — alt="" if decorative?
Does every form field have a real <label>?
Does the link text make sense out of context?
Is the DOM order the reading order? Anything reordered visually is a red flag.

2. The CSS. Whether it survives the next change.

What is the specificity? Anything above 0,2,0 needs a reason.
Any id selector? Any !important?
Are values tokens, or hard-coded hex and pixels?
Does the component know where it is — outer margin, width, position?
Are the class names about role or about appearance?
Is a magic number explained?

3. Responsiveness and content resilience. Covered by opening it, which is why that comes first.

4. Accessibility specifics. Contrast, focus rings, anything colour-only, ARIA that has been added rather than avoided.

5. Performance. Image sizes and dimensions, a lazy hero, a new font.

The UI smells list

Worth keeping. These are the things to look for in a diff, and most of them are one-line fixes at review time and expensive later.

HTML

A <div> or <span> with an onclick.
An <a> with no href, or href="#".
A <button> with no type inside a form.
A heading level chosen for size.
alt missing entirely, or starting with "image of".
A <div class="title"> styled as a heading.
A placeholder used as the only label.
<br><br> for spacing.
A list of items that is not a list; tabular data that is not a table.
A table used for layout.
tabindex with a positive number.
aria-* added where a native element would have done it.
aria-hidden on something focusable.
A new page with no unique <title>.

CSS

outline: none with no replacement.
!important.
An id in a selector.
A descendant chain three or more levels deep.
A hard-coded colour or spacing value where a token exists.
px for a font size.
A fixed height on something containing text.
width where max-width was meant.
min-width on something nested.
A missing min-width: 0 on a flex item, or 1fr without minmax(0, …).
Animating width, height, top or left.
Motion with no prefers-reduced-motion handling.
A position: fixed inside something with a transform.
z-index: 9999.
@import in production CSS.
:invalid rather than :user-invalid.
:focus rather than :focus-visible for a ring.

Assets

An image with no width and height.
loading="lazy" on the hero, or missing below the fold.
A PNG of a photograph.
An unresized photograph.
A new font family, or a third weight.
A font preload with no crossorigin.
An og:image missing on a new page.

That is most of this course, phrased as things to look for. It is worth reading through once before your first review.

The questions worth asking

Five, and they find more than the list does because they make the author think:

"What does this look like at 320px?" The commonest unexamined case.

"What happens with four times as much text?" The commonest real-world failure.

"How does this work with a keyboard?" Asked before a custom widget is built, it usually produces a native element instead.

"What does this say if you cannot see it?" Asked of an icon, a status colour or a chart, it produces the text that would otherwise be retrofitted.

"What happens when the design changes?" A .red-text class or a hard-coded hex answers itself.

Comments that land

Say the consequence, not the verdict, and label the severity — the same three words as the other courses:

blocking: this removes the focus ring with no replacement, so the form cannot be
completed with a keyboard at all.

blocking: the hero is 2.4 MB unresized. On Slow 4G that is about eleven seconds
before anything is visible.

question: at 320px does the price still fit next to the title, or does it wrap
under? I could not tell from the diff.

nit: `.blue-button` is green. `.button--primary`? Not blocking.

blocking, question, nit. A reviewer who does not label severity leaves the author guessing which of nine comments must be fixed.

Ask rather than assert when you have not opened it. "At 320px does the price wrap?" is better than "this breaks on mobile" when you have not checked, and half the time the answer teaches you something about the component.

Say what is good, specifically. Not "LGTM". "Putting the spacing on the container rather than the card means this works in the sidebar too" tells the author which instinct to repeat, and that is the only part of a review that improves the next pull request.

Screenshots, and why they are not enough

A screenshot in a pull request is genuinely useful — it is the fastest way to communicate what changed, and a before-and-after pair is better than a paragraph.

And it is one width, one colour scheme, one content length, with a mouse. It cannot show you the focus order, the 320px case, the long-name case, or the dark mode. Attach screenshots, review the preview URL.

What the tools should do first

So that the review is about design rather than mechanics:

Prettier for formatting, so nobody comments on indentation.

Stylelint for CSS — it catches duplicate properties, invalid values, unknown properties (the silent-typo problem from module 2), and can enforce a maximum specificity and ban !important outright:

{
  "rules": {
    "declaration-no-important": true,
    "selector-max-id": 0,
    "selector-max-specificity": "0,3,0"
  }
}

Those three lines remove three whole categories of review comment. Run against a file containing !important, an id selector and a four-class chain, they report exactly that:

2:29  Disallowed !important                                   declaration-no-important
3:1   Too many ID selectors in "#bad-id", maximum 0           selector-max-id
3:1   Too high specificity in "#bad-id", maximum "0,3,0"      selector-max-specificity
4:1   Too high specificity in ".a .b .c .d", maximum "0,3,0"  selector-max-specificity

html-validate or the W3C validator for markup — unclosed tags, invalid nesting, duplicate ids.

axe or Lighthouse in CI, so the mechanical accessibility third is caught automatically and the human review is about the other two thirds.

The payoff, as in every course: a reviewer who cannot comment on formatting will comment on the markup.

Reviewing your own work first

The cheapest review, and the one people skip.

Open your own pull request and read the diff on the platform, not in your editor where you have been looking at it for three hours. You will reliably find a commented-out rule, a console.log, an unresized image you meant to replace, a TODO, and a class you renamed in one place.

Then open your own preview URL and run the three-minute pass at the top of this lesson. Ten minutes, and the human review becomes about the design instead of the obvious.

Receiving a review

Every comment is about the code. If a reviewer misread your markup, that is usually a fact about the markup — the fix is a clearer class name or a comment saying why, not a reply explaining it.

Answer the ones you disagree with; "I chose this because…" is legitimate, and sometimes the reviewer changes their mind. And push fixes as new commits so the reviewer can see what changed, rather than force-pushing mid-review.

Check your work

Why UI review is different. No logic to trace and no test suite — the questions are about meaning, survivability and other people's browsing.

The first thing to do. Open the preview URL and use it for three minutes.

The three-minute pass. 320px, 1440px, keyboard, the test fixture, dark mode and reduced motion.

Why the diff is not enough. CSS is a system; a change to .card may break the card in the sidebar.

The order to read in. HTML, then CSS, then responsiveness, then accessibility, then performance.

Why HTML first. It carries the meaning, and meaning problems are the expensive ones.

The CSS threshold worth questioning. Anything above 0,2,0.

The five questions. 320px, four times the text, keyboard, what it says unseen, and what happens when the design changes.

The three severity labels. blocking, question, nit.

What a screenshot cannot show. Focus order, 320px, long content, dark mode.

Three Stylelint rules that remove whole categories of comment. declaration-no-important, selector-max-id, selector-max-specificity.

Why run tools first. A reviewer who cannot mention formatting will mention the markup.

What it means when a reviewer misread your markup. The markup needs to be clearer.

Practice

Open your own most recent change's preview URL and run the three-minute pass. Write down what you find.
Read your own diff on the platform rather than in your editor. List what you find that you had not noticed.
Take the UI smells list and go through one of your own pages against it. Count the hits.
Review a friend's page, or any open pull request on a public site, in the five-step order. Notice where you were tempted to start.
Ask the five questions of a component you built last week. Answer them honestly.
Rewrite three blunt review comments as a consequence plus a question, with labels.
Write one specific piece of praise about somebody's code — including your own.
Add Prettier and Stylelint to a project. Run Stylelint and count the findings.
Add the three Stylelint rules above and see how much of your own CSS fails.
Run your HTML through validator.w3.org and fix what it reports.
Add axe or Lighthouse to CI, or run it manually and record the result in the pull request.
Take a screenshot of a change, then list three things about it the screenshot cannot show.
Find a pull request on a public repository that changed CSS and judge whether the review asked any of the five questions.

Official documentation

Stylelint — Rules — Every rule, including the specificity and !important limits.
Stylelint — Getting started — Installing and configuring it.
W3C — Markup validation service — The authoritative HTML validator.
W3C WAI — Easy checks: a first review — A short manual accessibility review, which doubles as a review checklist.
Prettier — Options — Formatting, settled once.
web.dev — Lighthouse CI — Running the audits on every pull request.

You can now write HTML and CSS that somebody else can change: named for what things are, organised so specificity is never a fight, resilient to content you did not expect, fast on the phone your visitor actually has, and reviewable.

Next module: the capstone — a complete, responsive, accessible site for a real small business, built and deployed.