refactor: ui package style inconsistencies#3265
Conversation
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@ogzhanolguncu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 58 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough## Walkthrough
This update introduces reusable form helper components for labels and descriptions, refactors form input components to use these helpers, and adjusts focus ring styles across UI elements for consistency. Button and icon components are enhanced with improved accessibility, dynamic sizing, and animation. Documentation and example usage are updated for clarity and accessibility.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------------------------|---------------|
| `internal/ui/src/components/form/form-helpers.tsx` | Introduces new `FormLabel` and `FormDescription` components for consistent form labeling and helper text, with accessibility and variant support. |
| `internal/ui/src/components/form/form-tags.tsx` | Adds new `OptionalTag` and `RequiredTag` components for form field labeling, supporting error and warning styles. |
| `internal/ui/src/components/form/form-checkbox.tsx`<br>`internal/ui/src/components/form/form-input.tsx`<br>`internal/ui/src/components/form/form-textarea.tsx` | Refactors to use new `FormLabel` and `FormDescription` components, removing inline label/description logic and associated icon/tag imports. |
| `internal/ui/src/components/button.tsx` | Adds exported `DocumentedButtonProps` type, refines focus ring styling, removes width-locking logic for loading state, improves accessibility, and animates loading transitions. |
| `internal/ui/src/components/checkbox.tsx`<br>`internal/ui/src/components/input.tsx`<br>`internal/ui/src/components/select.tsx`<br>`internal/ui/src/components/textarea.tsx` | Updates focus ring styling: removes explicit ring widths, increases color intensity, and improves consistency across variants. |
| `internal/icons/src/icons/triangle-warning.tsx`<br>`internal/icons/src/icons/triangle-warning-2.tsx` | Refactors icon components to support dynamic sizing via a `size` prop and updates SVG structure and attributes for flexibility. |
| `apps/dashboard/components/ui/form-combobox.tsx` | Updates import path for `OptionalTag` and `RequiredTag` to new location. |
| `internal/ui/package.json` | Removes `lucide-react` dependency. |
| `apps/engineering/content/design/components/button.mdx` | Updates Button documentation and examples for clarity, accessibility, and consistency, including new `loadingLabel` prop in examples. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant FormComponent (Input/Checkbox/Textarea)
participant FormLabel
participant FormDescription
User->>FormComponent: Interacts (focus, input, etc.)
FormComponent->>FormLabel: Render label (with required/optional, error)
FormComponent->>FormDescription: Render description or error (with variant)
FormLabel-->>FormComponent: Labeled element
FormDescription-->>FormComponent: Helper or error textsequenceDiagram
participant User
participant Button
participant Spinner
User->>Button: Click or trigger loading
Button->>Spinner: Show loading spinner and label
Button-->>User: Animate transition (normal content <-> loading)
Suggested reviewers
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/engineering/content/design/components/button.mdx (1)
1043-1043: Consider alternative wordingThis line uses "very small screens" which contains a weak adjective that could be more specific.
- Consider stacking buttons vertically on very small screens + Consider stacking buttons vertically on mobile screens or small viewports🧰 Tools
🪛 LanguageTool
[style] ~1043-~1043: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...Consider stacking buttons vertically on very small screens(EN_WEAK_ADJECTIVE)
internal/ui/src/components/form/form-textarea.tsx (1)
42-48: Good extraction of label rendering to FormLabel component.Delegating label rendering to the shared FormLabel component improves consistency across form components.
However, note the small inconsistency in error handling: this component uses
!!errorwhile form-checkbox.tsx usesBoolean(error). Both are functionally equivalent, but consider standardizing on one approach for better code consistency.- hasError={!!error} + hasError={Boolean(error)}internal/ui/src/components/form/form-helpers.tsx (2)
3-4: Fix typo in inline comment
Reqired→Required.
Although minor, spelling errors in comments accumulate noise and make the codebase look less polished.-// biome-ignore lint/style/useImportType: Reqired for silencing Biome +// biome-ignore lint/style/useImportType: Required for silencing Biome
70-84: Enforce mutual exclusivity betweenrequiredandoptionalThe current API allows both props to be
true, which would render both tags and confuse users.
Consider asserting exclusivity either through the type system or runtime check.-export type FormLabelProps = { - label?: string; - required?: boolean; - optional?: boolean; +export type FormLabelProps = + | { label?: string; required: true; optional?: never; hasError?: boolean; htmlFor: string } + | { label?: string; optional: true; required?: never; hasError?: boolean; htmlFor: string } + | { label?: string; required?: false; optional?: false; hasError?: boolean; htmlFor: string };If you prefer runtime validation, throw when both props are truthy.
internal/ui/src/components/button.tsx (2)
435-436: Width can still shrink on first frame
min-w-maxsetsmin-width: max-content, but when the first render is the loading state, the max-content width is only the spinner, causing a noticeable jump once children appear.Add an inline style that stores the measured width on mount, or keep the old ref-measurement strategy behind a feature flag.
494-518: Avoid duplicating<style>tags on each importEvery import of
button.tsxappends the same<style>block todocument.head.
Guard with a module-level flag or inject once in a shared location to prevent unnecessary DOM bloat.-if (typeof document !== "undefined") { - const style = document.createElement("style"); - style.textContent = `…`; - document.head.appendChild(style); -} +declare global { + // eslint-disable-next-line no-var + var __unkey_button_styles_injected__: boolean | undefined; +} +if (typeof document !== "undefined" && !globalThis.__unkey_button_styles_injected__) { + const style = document.createElement("style"); + style.textContent = `…`; + document.head.appendChild(style); + globalThis.__unkey_button_styles_injected__ = true; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
apps/dashboard/components/ui/form-combobox.tsx(1 hunks)apps/engineering/content/design/components/button.mdx(41 hunks)internal/icons/src/icons/triangle-warning-2.tsx(1 hunks)internal/icons/src/icons/triangle-warning.tsx(2 hunks)internal/ui/package.json(0 hunks)internal/ui/src/components/button.tsx(18 hunks)internal/ui/src/components/checkbox.tsx(13 hunks)internal/ui/src/components/form/form-checkbox.tsx(2 hunks)internal/ui/src/components/form/form-helpers.tsx(1 hunks)internal/ui/src/components/form/form-input.tsx(3 hunks)internal/ui/src/components/form/form-tags.tsx(1 hunks)internal/ui/src/components/form/form-textarea.tsx(3 hunks)internal/ui/src/components/input.tsx(1 hunks)internal/ui/src/components/select.tsx(2 hunks)internal/ui/src/components/textarea.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- internal/ui/package.json
🧰 Additional context used
🧬 Code Graph Analysis (8)
internal/ui/src/components/form/form-checkbox.tsx (1)
internal/ui/src/components/form/form-helpers.tsx (2)
FormLabel(70-86)FormDescription(16-60)
internal/icons/src/icons/triangle-warning.tsx (1)
internal/icons/src/props.ts (2)
IconProps(30-35)sizeMap(7-28)
internal/ui/src/components/form/form-input.tsx (1)
internal/ui/src/components/form/form-helpers.tsx (2)
FormLabel(70-86)FormDescription(16-60)
internal/icons/src/icons/triangle-warning-2.tsx (1)
internal/icons/src/props.ts (2)
IconProps(30-35)sizeMap(7-28)
internal/ui/src/components/form/form-tags.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
internal/ui/src/components/form/form-helpers.tsx (4)
internal/icons/src/icons/triangle-warning-2.tsx (1)
TriangleWarning2(16-45)apps/dashboard/lib/utils.ts (1)
cn(5-7)internal/icons/src/icons/circle-info.tsx (1)
CircleInfo(15-56)internal/ui/src/components/form/form-tags.tsx (2)
RequiredTag(24-38)OptionalTag(11-22)
internal/ui/src/components/form/form-textarea.tsx (1)
internal/ui/src/components/form/form-helpers.tsx (2)
FormLabel(70-86)FormDescription(16-60)
internal/ui/src/components/button.tsx (1)
apps/dashboard/lib/utils.ts (1)
cn(5-7)
🪛 LanguageTool
apps/engineering/content/design/components/button.mdx
[style] ~1043-~1043: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...Consider stacking buttons vertically on very small screens
(EN_WEAK_ADJECTIVE)
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: autofix
🔇 Additional comments (31)
internal/ui/src/components/select.tsx (2)
4-4: Updated icon imports from custom packageThe icons import source was changed from
lucide-reactto@unkey/icons, which aligns with the PR objective to remove the lucide dependency from the UI package.
16-16: Improved focus ring styling consistencyThe focus ring styles have been updated across all variants to use a more consistent approach: replacing explicit ring width classes with the generic
focus:ringclass and adjusting color intensities for better visibility.Also applies to: 20-20, 24-24, 28-28
internal/ui/src/components/input.tsx (1)
12-12: Standardized focus ring stylingThe focus ring styles have been standardized to match the approach used in other components, using the generic
focus:ringclass paired with appropriate color intensities for each variant. This creates a more consistent focus state appearance across the UI library.Also applies to: 16-16, 20-20, 24-24
apps/dashboard/components/ui/form-combobox.tsx (1)
5-5: Updated import path for form tag componentsThe import path for
OptionalTagandRequiredTagwas updated to reference them from the new dedicated module@unkey/ui/src/components/form/form-tags. This change supports the extraction of these components for better reusability across form components.internal/ui/src/components/checkbox.tsx (2)
18-18: Standardized focus ring styles for primary checkbox variantsFocus ring styles for the primary, outline, and ghost variants have been updated to use a consistent approach with the generic
focus:ringclass and appropriate color intensities.Also applies to: 24-24, 30-30
61-61: Unified focus ring styling across color variantsThe focus ring styles have been standardized across all color variants (danger, warning, success, info) to create a consistent focus state appearance. The explicit ring width classes have been replaced with the
focus:ringclass, and the ring colors adjusted to lighter shades for better visibility.Also applies to: 71-71, 81-81, 92-92, 102-102, 112-112, 123-123, 133-133, 143-143, 154-154, 164-164, 174-174
internal/ui/src/components/textarea.tsx (2)
12-12: Consistent focus ring styling updatesThe focus ring styling has been updated to use a simpler approach without explicit numeric width classes, maintaining the visual feedback while improving consistency across components.
16-16: Improved focus ring color intensity for variant statesThe focus ring color intensity has been increased from
-2to-4for success, warning, and error states, which should provide better visual contrast when focused while maintaining the semantic meaning of each state.Also applies to: 20-20, 24-24
internal/icons/src/icons/triangle-warning.tsx (3)
15-18: Dynamic sizing support with standardized size mapThe component now properly implements the shared icon sizing system, importing the
sizeMapandIconPropsfrom the common props file. This enables dynamic sizing through thesizeprop with a sensible default of "xl-thin".
20-26: SVG attributes updated for dynamic sizingThe SVG element now properly receives dynamic height and width based on the selected size from the size map, while maintaining the same viewBox for consistent rendering.
34-34: Dynamic stroke width implementationThe stroke width is now dynamically set based on the selected size, ensuring visual consistency with other icons across the application.
Also applies to: 46-46
internal/ui/src/components/form/form-tags.tsx (3)
1-9: Well-structured component props with TypeScript typesGood implementation of TypeScript types for the component props. The
RequiredTagPropsproperly extends the baseTagPropswith the additionalhasErrorproperty for conditional styling.
11-22: Clean implementation of OptionalTag componentThe
OptionalTagcomponent is well implemented with consistent styling using the utilitycnfunction for class composition. The component allows customization through an optionalclassNameprop.
24-38: Robust implementation of RequiredTag with conditional stylingThe
RequiredTagcomponent properly implements conditional styling based on thehasErrorprop, using appropriate semantic colors for both error and warning states. The component maintains consistent spacing and typography with theOptionalTagcomponent.apps/engineering/content/design/components/button.mdx (4)
32-32: Improved accessibility with loadingLabelAdded
loadingLabel="Loading..."to the primary button loading example, which improves accessibility by providing a descriptive text for screen readers during loading states.
5-5: Improved document readability with consistent spacingThe added blank lines between sections and examples improve the document's readability and organization, making it easier to visually distinguish between different sections.
Also applies to: 37-37, 47-47, 77-77, 100-100, 131-131, 153-153, 191-191, 216-216, 250-250, 275-275, 309-309, 334-334, 368-368, 393-393, 427-427, 452-452, 486-486, 511-511, 545-545, 570-570, 604-604, 629-629, 663-663, 688-688, 722-722, 747-747, 781-781, 806-806, 840-840, 865-865, 885-885, 896-896, 927-927, 945-945, 984-984, 999-999
1008-1008: Updated AutoTypeTable to use DocumentedButtonPropsThe
nameprop forAutoTypeTablehas been changed from "ButtonProps" to "DocumentedButtonProps", reflecting the explicit type created in the button component for documentation purposes.
350-351: Consistent formatting for hover state button examplesThe className assignments for hover state buttons have been formatted consistently across all color variants, improving code readability and maintenance.
Also applies to: 375-376, 527-528, 552-553, 704-705, 729-730
internal/ui/src/components/form/form-checkbox.tsx (4)
4-4: Good addition of reusable form helpers.The import of
FormLabelandFormDescriptionfrom form-helpers supports the PR's goal of extracting common form component logic for better reusability.
54-54: Improved clarity with Boolean() constructor.Using
Boolean(error)instead of the double negation pattern (!!error) makes the code more explicit and self-documenting while maintaining the same functionality.
59-65: Good extraction of label rendering to FormLabel component.Delegating label rendering logic to the shared FormLabel component improves consistency across form components and reduces duplicate code.
68-74: Good extraction of description/error handling to FormDescription component.Moving the description and error rendering logic to the FormDescription component maintains the same functionality while improving code organization and reusability.
internal/ui/src/components/form/form-textarea.tsx (2)
4-4: Good addition of reusable form helpers.The import of
FormLabelandFormDescriptioncomponents aligns with the PR's goal of extracting common form component logic.
62-68: Good extraction of description/error handling to FormDescription component.Moving the description and error rendering logic to the FormDescription component improves code organization and reusability.
internal/icons/src/icons/triangle-warning-2.tsx (4)
15-18: Great enhancement for dynamic icon sizing.Adding support for dynamic sizing with the
sizeMapimproves flexibility and standardization across the UI library. The default size of "xl-thin" provides a sensible fallback.
20-26: Good update to SVG attributes for dynamic sizing.Updating the SVG to use dynamic dimensions from
sizeMapmakes the icon component more flexible. The viewBox change from "0 0 12 12" to "0 0 18 18" also updates the coordinate system for better alignment.
28-40: Good update to path elements with dynamic stroke width.The path elements now properly use dynamic stroke width from the size configuration, improving consistency with other icons.
41-41: Good addition of circle element for warning icon.The addition of the circle element completes the updated warning icon design, providing a more polished appearance.
internal/ui/src/components/form/form-input.tsx (3)
4-4: Good addition of reusable form helpers.The import of
FormLabelandFormDescriptioncomponents aligns with the PR's goal of extracting common form component logic.
26-32: Good extraction of label rendering to FormLabel component.Delegating label rendering to the shared FormLabel component improves consistency across form components and reduces duplicate code.
42-48: Good extraction of description/error handling to FormDescription component.Moving the description and error rendering logic to the FormDescription component maintains the same functionality while improving code organization and reusability.
|
the buttons are messed up now and have way too much padding |
|
more padding issues |
|
the alert when I click on details is gone, in prod I get a warning cause the key can't be shown again |
|
Last one not regression probably not up to date with main |
|
and for button I thought it would be a good idea to add some more default paddings, but its a little too late for that probably. I'm just gonna revert paddings |
2 participants
What does this PR do?
This PR fixes UI inconsistencies between our internal components. Mainly focuses on focus state.
And, make
FormXXXXcomponents more reusable by extracting common logic and components.Removes
lucidefrom ui packageType of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Documentation
Chores