Skip to content

feat(eslint-plugin): point plain text elements to Text element components - #667

Open
wemra3 wants to merge 9 commits into
mainfrom
feat/eslint-prefer-text-components
Open

wemra3 wants to merge 9 commits into
mainfrom
feat/eslint-prefer-text-components

Conversation

@wemra3

@wemra3 wemra3 commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Part of tailor-inc/platform-planning#2214. In the 2026-10-26 grooming and sync, Seiya asked for lint rules that guide consumer code to the new styles, and Sean said lint could be high priority. #651 added the first rule (no-deprecated-tokens). This PR adds the second one.

AppShell 1.19.0 added element components such as Text.p and Text.h3. The styling guide says not to style a plain p, h1 to h6, or span with text-sm or text-xs. Nothing checks this today.

Summary

  • New rule prefer-text-components. On a plain p, h1 to h6, or span element, it reports a Tailwind font size, and text styled by hand without a typography role. It names the element component and its default role.
    • <p className="text-sm"> → use <Text.p> (default role body-md)
    • <h3 className="text-lg font-semibold"> → use <Text.h3> (default role heading-sm), and set another role with variant if needed
    • <span className="text-xs"> → use <Text.span> (default role label-md), or a role utility such as text-label-md or text-body-sm when the app styles the element itself
  • What it reports (the second and third items were added after review):
    • A size on any of these elements: text-xs to text-9xl, arbitrary sizes in px, rem or em, and text-[length:...].
    • On h1 to h6 with no role utility: any weight, leading-* or tracking-*. For example, <h3 className="font-semibold"> → <Text.h3>. A heading has no use for inline emphasis.
    • On p and span with no role utility: leading-*, tracking-*, or a weight other than font-semibold. font-semibold alone stays allowed, because the typography decision record allows it for emphasis inside a sentence and not font-medium.
    • When the class string has a role utility, the other classes are not checked.
  • It reads only a class string written directly in className: "...", {"..."}, or a template literal without interpolation. It splits the string on whitespace, so text-sm is found among other classes. cn(...), ternaries, variables and constants, and template literals with interpolation are out of scope for now (changed after review).
  • No autofix. A size does not decide the role: text-sm can be body-md, label-md or heading-sm.
  • Not in recommended. Apps enable it in their own config. The rule reports many places in existing apps (see below), and Text.* exists only from AppShell 1.19.0. In an app on an earlier version, the rule would name components that do not exist yet. The README states this.
  • splitVariants moves from no-deprecated-tokens to a shared module. Its behavior does not change.
  • Docs: the plugin README, one sentence in the styling guide, and a "Fifth change" section in decisions/typography-roles.md.

Tried on the ERP-kit scaffold

I packed the plugin with pnpm pack, installed the tarball in a new project with oxlint 1.89.0 (the version the ERP-kit frontend uses), and used the config from the README. On packages/erp-kit/templates/scaffold in tailor-platform/erp-kit (origin/main, 3bcf8782), the rule reports 292 warnings in 97 files, and the exit code is 0:

  • Sizes: 264 (p 180, span 37, h3 33, h2 7, h1 5, h4 2)
  • Headings styled by hand: 12, for example <h3 className="font-semibold"> in the create forms and <h3 className="font-medium"> in the empty state
  • p and span styled by hand: 16. 15 use font-medium (card titles such as <p className="font-medium">, and author names in the revision history), and 1 is font-medium uppercase tracking-wide in language-switcher.tsx
  • No p or span with font-semibold alone is reported.

For text styled by hand, a separate script that does not use the rule finds the same 12 and 16, so there are no missed cases. I read all 28 of these reports and found no false positives. The size reports are the same 264 as on the earlier scaffold (fd4e9cd1), where I read all 84 reports on headings and span and found no false positives. I did not read each of the 180 p reports with a size. Other elements and components that set a size (Field.Label, div, Table.Head, ...) are out of scope.

Checks

  • packages/eslint-plugin: test (88 tests, including 46 for the new rule), lint, type-check and build pass. I broke the interpolation check, the role check and the heading weight check one at a time, and the tests failed each time.
  • recommended.test.ts checks that recommended does not report the rule, and that a config that enables it does.
  • Root fmt:check and docs:check pass. Changeset: minor for @tailor-platform/eslint-plugin-app-shell.

🤖 Generated with Claude Code

Hiroki Uemura and others added 2 commits October 31, 2026 17:06
…ents

Add prefer-text-components. It reports a Tailwind font size on a plain p,
h1 to h6, or span element and names Text.<element> with its default role.
For span it also names a role utility. It reads className strings, template
literals, ternary branches and cn/clsx/cx/twMerge/twJoin arguments, has no
autofix, and is not in recommended because Text.* needs AppShell 1.19.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nents

Read the arguments of classNames(), and unwrap as, satisfies and non-null
expressions. The span message now names a 14px role utility as well as a
12px one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wemra3
wemra3 requested a review from a team as a code owner November 1, 2026 09:12
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Code Metrics Report

main (f9acc22) #667 (50e55a9) +/-
Coverage 89.6% 89.6% 0.0%
Test Execution Time 3m46s 3m23s -23s
Details
  |                     | main (f9acc22) | #667 (50e55a9) | +/-  |
  |---------------------|----------------|----------------|------|
  | Coverage            |          89.6% |          89.6% | 0.0% |
  |   Files             |            269 |            269 |    0 |
  |   Lines             |           7728 |           7728 |    0 |
  |   Covered           |           6930 |           6930 |    0 |
+ | Test Execution Time |          3m46s |          3m23s | -23s |

Reported by octocov

Hiroki Uemura added 2 commits November 2, 2026 21:25
Resolve docs-manifest.json by regenerating with pnpm docs:sync.
…nto feat/eslint-prefer-text-components

# Conflicts:
#	docs-manifest.json

IzumiSy commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

I think we can initially limit this rule to string literals written directly in className and template literals without interpolation. collectStrings currently handles conditional expressions, class-name helpers, and other expression types, which adds more implementation and maintenance complexity than we need at this stage.

This does not mean limiting detection to a single class name. All of the following should be checked by splitting the string on whitespace and detecting text-sm among the classes:

<p className="text-sm hoge" />
<p className={"text-sm hoge"} />
<p className={`text-sm hoge`} />

We can document that cn(...), conditional expressions, variable or constant references, and template literals with interpolation are out of scope for now, and add support when needed.

…-text-components

Check a string literal, a string literal in braces, or a template literal
without interpolation. cn() and other class helpers, ternaries, variables
and interpolated templates are out of scope for now. The erp-kit scaffold
still gives 264 reports in 91 files.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wemra3

wemra3 commented Nov 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. I narrowed the rule as you suggested in 6f66ac3.

  • It now reads only a string literal, a string literal in braces, or a template literal without interpolation. It splits the string on whitespace, so the three forms in your comment are reported. Tests cover each of them.
  • cn(...), ternaries, variables and constants, and template literals with interpolation are not checked. The README and the decision record say they are out of scope for now.
  • On the erp-kit scaffold, the result does not change: 264 reports in 91 files. 355 of the 356 className values on these elements are plain strings.

🤖 Addressed by Claude Code

…nto feat/eslint-prefer-text-components

# Conflicts:
#	docs-manifest.json

@interacsean interacsean left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @wemra3 — I am wondering if this can be expanded a little bit further for other cases where consumers are using a utility class on an html element that could be replaced by a text component.

Claude suggested:

h1 to h6: report any weight, leading-* or tracking-* when the class string has no role utility. A heading has no inline-emphasis use, so false positives should be close to zero. This needs a second message, because there’s no size to quote in found.
span / p with tracking-* or leading-* (and no role utility): worth reporting too. In the scaffold, font-medium uppercase tracking-wide on a span in language-switcher.tsx is an eyebrow label, i.e. a label role built by hand.
span / p with only a weight: keep these allowed. inside a sentence is real emphasis (9 spans in the scaffold, mostly in revision-history drawers). The 7

elements look like label-md, but they’re ambiguous, so leave them out of v1.
Test: flip test line 29 to an invalid case.

Hiroki Uemura and others added 3 commits November 6, 2026 21:44
…-text-components

When the class string has no typography role, report any weight,
leading-* or tracking-* on h1 to h6, and leading-*, tracking-* or a
weight other than font-semibold on p and span. font-semibold alone stays
allowed on p and span as emphasis inside a sentence. On the erp-kit
scaffold this adds 12 headings and 16 p and span elements (292 in all).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ext-components

Report font-[600] like a named weight, decide headings with /^h[1-6]$/,
and word the p/span message so the font-semibold note fits every report.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wemra3

wemra3 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the proposal. I checked it on the latest erp-kit scaffold and added it in this PR, with one change for p and span.

  • h1 to h6 with no role utility: any weight, leading-* or tracking-* is reported, with a second message. This adds 12 reports, such as <h3 className="font-semibold"> in the create forms.
  • p and span with no role utility: leading-* and tracking-* are reported, as you suggested. A weight other than font-semibold is also reported. The typography decision record allows only font-semibold for emphasis inside a sentence, not font-medium. All 15 weight-only cases in the scaffold use font-medium. The p ones are card titles that match label-md, and the span ones would be font-semibold if they are emphasis. font-semibold alone stays allowed.
  • The test at line 29 is now an invalid case.

The scaffold result is now 292 reports in 97 files (264 sizes, 12 headings, 16 p and span). I read the 28 new reports and found no false positives. The PR description has the details.

🤖 Addressed by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants