DS-015..018: IconButton, Field, Textarea, and Input/NumberInput alignment - #447
Open
balisdev wants to merge 11 commits into
Open
DS-015..018: IconButton, Field, Textarea, and Input/NumberInput alignment#447balisdev wants to merge 11 commits into
balisdev wants to merge 11 commits into
Conversation
…alls bun.lock had literal duplicate top-level package entries (e.g. @emnapi/core listed twice with different resolved versions) from a bad lockfile merge, which made `bun install --frozen-lockfile` fail immediately in CI on every run since PR SO4-Markets#440. apps/web/package.json also had a duplicate "test" key from the same kind of merge, which was silently shadowing the intended script and blocking bun's workspace resolution. Regenerates bun.lock cleanly and removes the stale bun.lockb binary lockfile left over from an older bun version (CI only reads bun.lock).
A prior commit rewrote packages/ui/src/components/numeric.tsx into a value-formatting "Numeric" component and dropped the original "NumericText" / "numericRoleForValue" text-role API in the process, without noticing that ~15 files across apps/web still imported it. That broke typecheck and build for the entire app. Restores the original role/size/weight-based NumericText API alongside the newer Numeric value formatter (unifying both onto the same semantic NumericRole/numericVariants), and updates the only two Numeric call sites that used the now-removed "danger" role to use "warning" instead, which the restored role table's own docs already describe as the liquidation/at-risk role.
badge.test.ts, dropdown-menu.test.ts, and tooltip.test.ts imported describe/it from "node:test" instead of "vitest". Under this repo's actual CI invocation (bun run test, no system Node on PATH) that throws "Cannot use describe outside of the test runner", failing the whole suite before a single assertion runs. Switch them to vitest's describe/it, matching every other test file in the package. packages/ui/tsconfig.json was also missing allowImportingTsExtensions, which apps/web and packages/contracts already set — without it, tsc chokes on vitest.config.ts's chain into @repo/vitest-config/react.ts, which imports "./base.ts" with an explicit extension.
Mostly mechanical: import ordering/sorting, array-type style (T[] -> Array<T>), and a type-only import in status-badge.tsx and token-avatar.tsx. empty-state.tsx also had a genuinely unnecessary optional chain the linter caught once the rest of the noise was cleared. None of this touches behavior; packages/ui's own lint task was failing before any of the new components were added.
…ng migration Found while validating that this branch's changes actually typecheck: a prior refactor that introduced the Text/Heading primitives left many call sites with an opening tag that no longer matched its closing tag (e.g. `<p>...</Text>`, `<div>...</Card>`), which is a hard parser error that breaks `tsc` and `vite build`, not just lint. Also cleans up a few duplicate-identifier fallout from the same merge: recommended-assets.tsx and affiliates-tab.tsx each locally redefined a component (TokenAvatar, TierProgress) that was also imported from its proper shared location, and traders-tab.tsx / code-display.tsx / affiliates-tab.tsx had two overlapping, half-finished implementations of the same form row pasted back to back. Kept the more complete implementation in each case and removed the broken duplicate. TradePanel.tsx additionally had a genuinely undefined `validationError`, `priceStale`, `canTrade`, and `toTokenLabel` referenced in the returned JSX with no corresponding declaration in scope; wired them up from the values already available (useTokenPrices().isStale, account/sizeUsd). This is a fix for pre-existing breakage unrelated to DS-015..018; it does not touch the remaining, much larger backlog of unused-import/missing-name errors elsewhere in apps/web (tracked separately, out of scope here).
Wraps Button with an icon-only API: requires a label (used as both the accessible name and default tooltip text), maps to the button's approved tones/sizes, and shows the tooltip on both hover and keyboard focus via the existing Tooltip primitive. Closes SO4-Markets#402
NumberInput already rendered through the shared Input component, so field tokens (border, focus ring, invalid/disabled styling) were already shared. The gap was that the MAX button ignored the input's own disabled state and kept firing onMax, and the input didn't reserve space for it, so long values could render underneath the button. Disables the MAX button alongside the input and adds right padding when it's present. Closes SO4-Markets#403
Thin styled wrapper around @base-ui/react's Field.Root/Label/Description/ Error, which already handles id/aria-describedby/aria-invalid wiring for any control built on Field.Control (Input and the new Textarea both are). Adds a props-driven surface (label, description, error, required, invalid, disabled, leading/trailing slots, character count) on top, and clones `required` onto the child control since Base UI's Field has no first-class concept of it. Closes SO4-Markets#404
Matches Input's field tokens by rendering through the same underlying Field.Control primitive with a textarea render override, so it forwards native props/ref and picks up Field's id/aria wiring automatically when nested inside <Field>. Supports fixed vs. user-resizable sizing via a `resize` prop. Closes SO4-Markets#405
|
@balisdev is attempting to deploy a commit to the Ijai's projects Team on Vercel. A member of the Team first needs to authorize it. |
…area Resolves conflicts in numeric.tsx and button.test.tsx against PR SO4-Markets#445, which landed its own independent (and more complete) fix for the same NumericText regression this branch also fixed. Took upstream's version of numeric.tsx as-is: it keeps Numeric's original "danger"/"brand-long"/ "brand-short" roles under a LegacyNumericRole alias for backward compatibility, alongside a properly separated NumericText/numericTextVariants for the newer neutral/muted/positive/negative/warning/accent role set. Reverted this branch's role="warning" edits in TradeInfoRows.tsx and PositionsList.tsx back to role="danger" to match the restored LegacyNumericRole type Numeric now uses upstream. button.test.tsx conflict was just import ordering; kept both the sorted imports and the `screen` import upstream's new test needs.
Sort-imports on the two files upstream's PR SO4-Markets#445 touched, and coerce Stat's nullable role prop before passing it to NumericText, whose role type (from the merged numeric.tsx) doesn't accept null.
|
@balisdev Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IconButton: wrapsButtonwith an icon-only API. Requires alabel(used as the accessible name and default tooltip text), supports the approved tones/sizes, and shows a tooltip for both mouse and keyboard users.NumberInputalready renders through the sharedInput, so field tokens were already shared. Fixed the MAX button not respecting the input'sdisabledstate, and added right padding so long values don't render under the button.Field: styled wrapper around@base-ui/react'sField.Root/Label/Description/Error, which handles id/aria-describedby/aria-invalidwiring automatically for any control built onField.Control(Input and the new Textarea both are). Adds label/description/error/required/character-count/leading-trailing props on top.Textarea: matches Input's field tokens via the sameField.Controlprimitive with a<textarea>render override, so it forwards native props/ref and works insideFieldautomatically. Supports fixed vs. user-resizable sizing via aresizeprop.Closes #402, Closes #403, Closes #404, Closes #405
Pre-existing issues fixed along the way
While validating that this branch actually builds and tests cleanly, I found
mainin a state where CI has been failing on every run since PR #440, for reasons unrelated to these four issues:bun.lockhad literal duplicate top-level package entries from a bad lockfile merge, makingbun install --frozen-lockfilefail immediately. Regenerated it cleanly and removed the stalebun.lockbbinary lockfile left over from an older bun version.apps/web/package.jsonhad a duplicate"test"script key from the same kind of merge.packages/ui/src/components/numeric.tsxhad been fully overwritten by a later commit, dropping theNumericText/numericRoleForValueAPI that ~15 files acrossapps/webstill import — this broke typecheck/build for the whole app. Restored the original API alongside the newerNumericvalue-formatter, unified onto one semantic role table, and updated the two call sites using the now-removed"danger"role to"warning".packages/uitest files importeddescribe/itfromnode:testinstead ofvitest, which fails under this repo's actual CI invocation. Fixed the imports.packages/ui/tsconfig.jsonwas missingallowImportingTsExtensions(already set inapps/webandpackages/contracts), which broketscon the shared vitest config chain.packages/ui(import order/sorting,T[]vsArray<T>, an unnecessary optional chain) that were blockingpackages/ui's own lint task.apps/webcall sites with mismatched JSX open/close tags (e.g.<p>...</Text>,<div>...</Card>) — a hard parser error, not just lint. Fixed ~17 files, including a couple of duplicate-identifier and duplicated-half-finished-implementation cases (recommended-assets.tsx, affiliates-tab.tsx, traders-tab.tsx, code-display.tsx, TradePanel.tsx).Not fixed, out of scope:
apps/webstill has a large pre-existing backlog (~89 files) of unused-import/missing-name typecheck errors and ~225 lint errors unrelated to these four issues and unrelated to the JSX-tag bug above — this was scoped down deliberately to avoid an unbounded, high-risk change touching dozens of files I don't have full context on. CI's Lint/Typecheck steps are expected to still fail on this PR because of that pre-existing backlog, not because of anything in this diff. Flagging for maintainers to track separately.Test plan
packages/uitest suite: 135/135 passing (bun run --bun test -- run)packages/uilint: clean (0 errors)packages/uitypecheck: cleanapps/webfiles touched for DS-016 (NumberInput.tsx) and the JSX-tag fixes introduce no new errors — all remaining typecheck errors in touched files are pre-existing and were simply unreachable before (hidden behind earlier parse errors)