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 ↗︎
|
📝 WalkthroughWalkthroughThis pull request removes several example button components and updates documentation to provide a more detailed overview of button variants, states, and features. A new button with keyboard shortcut support is introduced, and the main button component is refactored to include new styling variants, accessibility attributes, and loading behavior. In addition, CSS and Tailwind configurations are revised with expanded color variables, and various dashboard dialog and form components are refactored to use a unified dialog container. Minor updates include icon additions, improvements in number formatting, and simplified input focus styling. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as ButtonWithKeyboardShortcut
participant C as Counter
U->>B: Press Command/Ctrl+B
B->>C: Increment counter
sequenceDiagram
participant U as User
participant Btn as Action Button
participant DC as DialogContainer
participant Form as Form Handler
U->>Btn: Clicks to open dialog
Btn->>DC: Sets isOpen = true
DC->>Form: Renders form content
U->>Form: Populates and submits form
Form->>DC: Submission success callback
DC-->>U: Closes dialog (isOpen = false)
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
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: 1
🧹 Nitpick comments (15)
apps/engineering/content/design/components/button.tsx (1)
1-22: Consider blocking the default browser shortcut or verifying the user intent.
By default, some browsers may already use certain Cmd/Ctrl + key shortcuts. If overriding a system or browser shortcut is not intended, consider callinge.preventDefault()to avoid conflicting actions or let users know they’re overriding a default shortcut. Otherwise, the new feature logic and rendering approach look good.internal/ui/tailwind.config.js (4)
19-20: Revisit the drop shadow for better consistency.
A shadow with zero offsets and a 3px spread may produce a subtle outline effect. Verify that this meets your design guidelines and user experience expectations.
21-41: Evaluate necessity of extensive opacity values.
This wide range of opacity steps (in increments of 5) can be helpful, but ensure they are all truly needed to avoid cluttering your utility classes.
48-63: Provide a fallback strategy for missing color variables.
getColoris well-structured, but consider how you want to handle cases wherecolorVaris invalid or undefined—adding a default, try-catch, or a console warning can help with diagnosing color configuration mistakes.
65-88: Double-check alpha-color coverage for all relevant color sets.
Currently, only a few color sets have alpha variants. If your design specs require alpha shades for other colors (e.g., info, accent), consider adding them for consistency.apps/engineering/content/design/components/button.mdx (5)
2-3: Add a concise version name or broader context to the title.While the title and description are clear, consider specifying "Button v2" in the title or providing a short versioning note to help distinguish from the previous button documentation if both exist concurrently.
21-33: Use caution with Tailwind's!importantclasses.Applying
!bg-accent-12/90and!ring-4 !ring-gray-7can override other variant or state behaviors, which might lead to style inconsistencies in certain contexts. Consider scoping these overrides to necessary usage only.
170-203: Ensure caution with partial destructive states.While "Danger - Ghost" or "Warning - Primary" states are visually distinct, be mindful that minimal emphasis on destructive actions (e.g., ghost variant) can lead to accidental clicks. Ensure user confirmation flows are implemented if needed.
355-360: Add examples with icons in each size variant (optional).Showing icons across size variants can help users understand how icons scale. This can reduce confusion among consumers of the button component.
433-433: Avoid intensifiers like "very" in documentation.The phrase "Consider stacking buttons vertically on very small screens" might be simplified to "on small screens" or "on extremely small screens" for more precise context.
-Consider stacking buttons vertically on very small screens +Consider stacking buttons vertically on small screens🧰 Tools
🪛 LanguageTool
[style] ~433-~433: 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/button.tsx (2)
13-30: Consider grouping or commenting compound variants for clarity.Large arrays of strings make it hard to maintain or extend variants. Grouping them with inline comments or subdividing them by sub-variant (e.g., "base styles," "disabled states," etc.) can maintain clarity.
307-318: Extract reduced-motion logic for reuse.Embedding the reduced-motion preference style injection here is convenient, but placing it in a separate module or global CSS might be more maintainable, especially if multiple components require a similar pattern.
// Button.tsx -if (typeof document !== "undefined") { - const style = document.createElement("style"); - style.textContent = `...`; - document.head.appendChild(style); -} +// Proposed approach: separate a .css or .ts file with a global style that handles prefers-reduced-motion, +// then import it once at a higher level (e.g., _app.tsx in Next.js or your global stylesheet).internal/ui/colors.css (3)
25-37: Ensure consistent naming or grouping forGrayA (Slate A).You’ve labeled them "GrayA" but commented them as “(Slate A).” For clarity, confirm whether this naming convention is desired or if merging them into "grayA" or "slateA" is more consistent.
67-79: Verify color contrast for success shades.The new alpha-based success shades should be tested to meet WCAG AA minimum contrast ratios on various backgrounds.
Please run a script or external check to confirm color contrast for updated success shades.
210-222: Maintain consistent naming across dark theme overrides.You’re reusing
grayAin the dark theme as “SlateA,” which might cause naming confusion if you also refer to slate-based colors differently. Double-check that references remain coherent for theming across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/engineering/content/design/components/button.disabled.tsx(0 hunks)apps/engineering/content/design/components/button.icons.tsx(0 hunks)apps/engineering/content/design/components/button.keyboard.tsx(0 hunks)apps/engineering/content/design/components/button.loading.tsx(0 hunks)apps/engineering/content/design/components/button.mdx(1 hunks)apps/engineering/content/design/components/button.onClick.tsx(0 hunks)apps/engineering/content/design/components/button.shape.tsx(0 hunks)apps/engineering/content/design/components/button.tsx(1 hunks)apps/engineering/content/design/components/button.variant.tsx(0 hunks)internal/ui/colors.css(9 hunks)internal/ui/src/components/button.tsx(2 hunks)internal/ui/src/index.ts(1 hunks)internal/ui/tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (7)
- apps/engineering/content/design/components/button.icons.tsx
- apps/engineering/content/design/components/button.keyboard.tsx
- apps/engineering/content/design/components/button.loading.tsx
- apps/engineering/content/design/components/button.shape.tsx
- apps/engineering/content/design/components/button.onClick.tsx
- apps/engineering/content/design/components/button.disabled.tsx
- apps/engineering/content/design/components/button.variant.tsx
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/design/components/button.mdx
[style] ~433-~433: 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 (18)
- 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: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
internal/ui/tailwind.config.js (1)
15-15: Dynamic color generation looks good.
UsinggenerateRadixColors()is a solid way to maintain a centralized, scalable color system. This extends Tailwind’s capabilities without adding unnecessary complexity.apps/engineering/content/design/components/button.mdx (2)
387-391: Confirm keyboard shortcut usage.The example snippet is informative. If showing advanced usage, consider also displaying how to customize the
triggerfunction for other keys or combos.
6-10:Details
❓ Verification inconclusive
Confirm usage of the newly imported modules.
The
RenderComponentWithSnippetandButtonimports look appropriate, but verify that no references to the old example components remain in the docs or codebase to avoid confusion or dead imports.
🏁 Script executed:
#!/bin/bash # Description: Search for references to old example components in the codebase rg "DisabledExample|IconsExample|KeyboardExample|LoadingExample|OnClickExample|ShapeExample"Length of output: 94
Action: Verify that no deprecated example components remain in the codebase.
The new imports for
RenderComponentWithSnippetandButtonappear correct. However, our automated search for references to old example components (e.g.,DisabledExample,IconsExample,KeyboardExample,LoadingExample,OnClickExample,ShapeExample) did not return any matches. Given that the automated output was minimal and potentially inconclusive, please perform a manual review of the documentation and relevant source files to ensure these deprecated components are not referenced anywhere else.internal/ui/src/components/button.tsx (2)
9-9: Validate layering of CSS classes.The default class string for
buttonVariantsincludes multiple utility classes with potential overlap (e.g.,group,inline-flex,transition-colors, etc.). Ensure there's no unintended conflict with custom states, especially when combining with companion libraries or advanced theming systems.
203-216: Commendable accessibility approach witharia-disabledandaria-busy.Your approach ensures that screen readers and other assistive technologies correctly interpret button states. This is a best practice for inclusive design.
internal/ui/colors.css (1)
109-121: Cross-check partial transparency in the AmberA series.Transparent overlays, especially in UI overlays or backgrounds, can stack in complex ways. Confirm that alpha layering remains visually clear when combined with other brand or background colors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/ui/src/components/button.tsx (2)
50-144: Comprehensive compound variants but consider DRYer approachThe compound variants implementation is thorough and addresses all combinations of variant and color, which is excellent. However, there's significant repetition in the focus styles, border-radius values, and hover states across variants.
Consider extracting common styles into shared variables or helper functions to reduce repetition. For example:
+ // Define common focus styles by color + const getFocusStyles = (color: string) => `focus:border-${color}-11 focus:ring-4 focus:ring-${color}-7 focus-visible:outline-none focus:ring-offset-0`; + + // Define common disabled styles by variant + const getDisabledStyles = (variant: string) => variant === 'primary' + ? 'disabled:bg-$color-7 disabled:text-white/80' + : 'disabled:text-$colorA-7'; // Then in your compound variants: { variant: "primary", color: "danger", className: [ "text-white bg-error-9 hover:bg-error-10 rounded-md font-medium focus:hover:bg-error-10", - "focus:border-error-11 focus:ring-4 focus:ring-error-7 focus-visible:outline-none focus:ring-offset-0", + getFocusStyles("error"), "disabled:bg-error-6 disabled:text-white/80", "active:bg-error-11", ], },This approach would make the code more maintainable as changes to focus styles would only need to be made in one place.
267-270: Consider animating the loading indicator with CSS instead of classWhile the
animate-spinclass works, consider using CSS animations that can be more easily controlled with the prefers-reduced-motion preference. This would allow for a more direct connection between the animation and the reduced motion preference.<Loader - className="animate-spin" + className="loader-spin" data-prefers-reduced-motion="respect-motion-preference" /> + // Add this CSS to your styles: + /* + .loader-spin { + animation: spin 1s linear infinite; + } + + @media (prefers-reduced-motion: reduce) { + .loader-spin { + animation: none; + } + } + + @keyframes spin { + from { + transform: rotate(0deg); + } + to { + transform: rotate(360deg); + } + } + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/ui/src/components/button.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
internal/ui/src/components/button.tsx (11)
9-30: Well-structured button variants implementation!The reorganization of button variants using arrays of classes improves readability and maintainability compared to long string concatenations. This approach makes it easier to understand which styles apply to each variant state.
32-37: Good implementation of color system with semantic variantsAdding color variants (default, success, warning, danger) alongside the existing variant types creates a more flexible and semantically meaningful button system. This separation of concerns between the button's visual style (variant) and its semantic meaning (color) follows best practices for component design.
Also applies to: 46-48
175-178: Great accessibility improvement with loadingLabelAdding the
loadingLabelproperty enhances screen reader accessibility during loading states. This is an excellent practice that improves the user experience for users with disabilities.
181-200: Well-implemented keyboard shortcut indicator stylingThe
keyboardIconVariantsfunction provides a consistent and visually appealing way to style keyboard shortcut indicators. The variants appropriately match the button variants and provide good visual differentiation.
213-216: Good separation of concerns for disabled statesSeparating visual disabled state from click disabled behavior is an excellent approach. This allows the button to maintain its appropriate visual state while preventing interaction during loading, which improves both usability and accessibility.
218-229: Great UX enhancement with width preservation during loadingUsing a ref to maintain consistent button width during loading states prevents layout shifts, which significantly improves the user experience. This is a thoughtful detail that prevents surrounding content from moving when buttons toggle between loading and normal states.
231-247: Improved keyboard handler with disabled state considerationThe keyboard handler now correctly considers the disabled state, which prevents keyboard shortcuts from triggering when the button is disabled or loading. This is an important accessibility and UX improvement.
252-258: Enhanced accessibility with proper ARIA attributesUsing
aria-disabledfor click-disabled states andaria-busyfor loading states improves accessibility for screen reader users. The separation between the HTMLdisabledattribute and ARIA attributes provides better control over visual and functional states.
261-271: Accessible loading state implementationThe loading state implementation now includes proper aria-hidden attributes and a screen reader-only message, which greatly improves accessibility. The centered loading indicator with animation provides clear visual feedback.
284-294: Improved keyboard shortcut indicatorThe keyboard shortcut indicator now uses the
keyboardIconVariantsfunction for consistent styling based on the button variant. The use of the<kbd>element is semantically correct for keyboard indicators.
300-311: Excellent accessibility feature for respecting reduced motion preferencesAdding support for reduced motion preferences shows great attention to accessibility concerns. This implementation correctly respects the user's system preferences and will disable animations for users who have indicated a preference for reduced motion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/ui/src/components/button.tsx (5)
8-31: Consider extracting repeating styles in variant definitions.
Your approach is clear, but repeated styles inprimary,outline,ghostmight be refactored into shared classes or utility functions for easier maintenance in the future.
43-56: Backward compatibility note foriconandsquare.
Keeping these in the size/shape variants for backward compatibility is fine. Just ensure to track their eventual removal so that they don't accumulate technical debt.
57-151: Repeated compound variants for danger, warning, success.
All sets of compound variants share common focus styles, border definitions, etc. Consider consolidating repetitive styles to simplify future updates.
223-284: Animated spinner implementation is functional but could consider alternative approaches.
Using setInterval for rotation is fine; however, a CSS-only spinner or a single rotation animation might simplify the logic and reduce overhead. Nonetheless, the current approach works.
419-444: Inline style injection for reduced motion and spin animation.
This works, but using external or modular CSS might be more maintainable long-term, especially in SSR environments wheredocumentmight be undefined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/create-api-button.tsx(4 hunks)apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx(3 hunks)apps/dashboard/app/(app)/settings/team/invite.tsx(5 hunks)apps/dashboard/components/navigation/navbar.tsx(4 hunks)internal/ui/src/components/button.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/dashboard/app/(app)/settings/team/invite.tsx
- apps/dashboard/app/(app)/apis/create-api-button.tsx
- apps/dashboard/components/navigation/navbar.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx (3)
3-8: Multi-line import looks good.
No functional issue spotted with splitting the import statements for readability.
37-37: No functional change in the regex validation.
The reformatted error message remains correct. No concerns here.
67-71: Ensure consistency ofcolor="default"usage.
Usingcolor="default"aligns with your updated Button color variants. This should render the default styling as expected.internal/ui/src/components/button.tsx (6)
37-42: Potential brand alignment check fordangerand other color tokens.
Although the color variant keys are descriptive, confirm that “danger,” “success,” and “warning” align with design/brand standards for consistent naming across the project.
182-185: Good accessibility improvement withloadingLabel.
Including a screen reader-only label for the loading state is a best practice that enhances accessibility.
188-207: Keyboard icon styling meets the new variant system.
ThekeyboardIconVariantsfunction is well-structured. Ensure color contrast is sufficient for all variants.
286-300: Bounds check for getPathForSegment function.
Consider handling out-of-rangeindexgracefully or ensuring it’s never called with unexpected values to avoid potential runtime errors.
368-379: Distinguishing click-disabled from visual-disabled is a nice touch.
This approach avoids losing focus styling or pointer events while maintaining intended behavior. Great solution for nuanced states.
383-391: Overlaid spinner preserves button dimensions.
Hiding the children under a spinner overlay helps maintain layout stability during loading. Good implementation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/ui/src/components/button.tsx (1)
414-438: Good implementation of reduced motion supportAdding CSS for respecting reduced motion preferences is an excellent accessibility enhancement. The approach of injecting styles dynamically ensures the styles are available when the component is used.
However, consider moving this to a more centralized location like a global CSS file to:
- Avoid multiple injections if the component is used multiple times
- Keep the component logic focused on rendering
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/dashboard/app/(app)/apis/_components/create-api-button.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx(1 hunks)apps/dashboard/app/(app)/settings/team/invite.tsx(1 hunks)apps/dashboard/components/navigation/navbar.tsx(1 hunks)internal/ui/src/components/button.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/dashboard/app/(app)/ratelimits/_components/create-namespace-button.tsx
- apps/dashboard/app/(app)/settings/team/invite.tsx
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/dashboard/components/navigation/navbar.tsx (1)
74-77: Good update to the Button stylingThe changes to use
variant="outline"and addbg-gray-4 hover:bg-gray-5classes align well with the updated Button component design system. This creates better visual separation between the icon button and surrounding content, improving the UI consistency.apps/dashboard/app/(app)/apis/_components/create-api-button.tsx (1)
67-67:Details
❓ Verification inconclusive
Good adoption of the new Button color prop
Adding
color="default"to the Button properly utilizes the new component API. This ensures compatibility with the updated button design system while maintaining the primary variant style.You might want to also check other buttons in this component (like the one at line 97-105) to ensure consistent application of the color prop across all buttons in this file.
🏁 Script executed:
#!/bin/bash # Find all Button components in this file and check if they use the color prop cat "apps/dashboard/app/(app)/apis/_components/create-api-button.tsx" | grep -A 10 "<Button"Length of output: 1021
Consistent Button Color Prop Usage
The updated primary button at line 67 correctly includes
color="default", aligning with the new design system. However, the secondary submit button (lines 97–105) does not have acolorprop applied. Please verify whether this omission is intentional. If consistency is desired across the component, consider addingcolor="default"(or the appropriate color) to the second button.
- File: apps/dashboard/app/(app)/apis/_components/create-api-button.tsx
- Line 67:
<Button variant="primary" {...rest} color="default">…- Line 97–105:
<Button type="submit" variant="primary" …>(missing the color prop)internal/ui/src/components/button.tsx (10)
7-9: Good improvements to the button base stylingThe updated base styles provide a more solid foundation with better transitions and focus states, ensuring consistent behavior across all button variants.
12-31: Good restructuring of button variantsThe reorganization of button variants with clear, semantic names (primary, outline, ghost) improves the design system's consistency. The detailed styling for each variant with proper focus, hover, and active states enhances the user experience.
37-42: Well-implemented color system for buttonsAdding the color variants (default, success, warning, danger) allows for more contextual use of buttons across the application, improving user understanding of actions.
44-49: Good button sizing optionsThe updated size variants with clear naming conventions (sm, md, lg, xlg) and the appropriate heights make it easier to maintain visual consistency across the application.
57-151: Excellent implementation of compound variantsThe compound variants system elegantly combines colors and variants, providing a comprehensive set of button styles that maintain consistent design language while offering contextual variations. The detailed styling for different states (hover, focus, disabled, active) ensures a polished user experience.
182-186: Good addition of screen reader supportAdding the
loadingLabelprop enhances accessibility by providing context to screen reader users during loading states.
188-207: Well-designed keyboard shortcut indicatorThe
keyboardIconVariantsimplementation provides consistent styling for keyboard shortcuts across different button variants, enhancing the UI for power users.
219-294: Excellent implementation of animated loading spinnerThe
AnimatedLoadingSpinnercomponent provides:
- A visually appealing loading indicator with sequential animation
- Proper SVG implementation with good performance
- Support for reduced motion preferences (line 250)
This is a significant improvement over a simple spinner and provides better visual feedback to users.
307-338: Robust variant and width management during loadingThe implementation properly handles:
- Mapping old variants to new ones for backward compatibility
- Preserving button width during loading state to prevent layout shifts
- Separate handling of visual and functional disabled states
This provides a smoother user experience, especially during async operations.
371-373: Consider enhancing accessibility for loading stateWhile using
aria-busy={loading}is a good start, this attribute has inconsistent support across screen readers. Consider adding a live region announcement for loading state changes.For more reliable screen reader announcements, consider adding a visually hidden live region:
const Button: React.FC<ButtonProps> = ({ // existing props }) => { // existing code + // Live region for loading state announcements + React.useEffect(() => { + if (loading) { + // Create or update live region when loading starts + let liveRegion = document.getElementById('button-loading-status'); + if (!liveRegion) { + liveRegion = document.createElement('div'); + liveRegion.id = 'button-loading-status'; + liveRegion.setAttribute('aria-live', 'polite'); + liveRegion.className = 'sr-only'; + document.body.appendChild(liveRegion); + } + liveRegion.textContent = loadingLabel; + + return () => { + // Clear announcement when loading ends + if (liveRegion) { + liveRegion.textContent = ''; + } + }; + } + }, [loading, loadingLabel]); // rest of the component }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/dashboard/app/(app)/ratelimits/_components/controls/components/logs-datetime/index.tsx (1)
80-82: Consider enhancing keyboard shortcut accessibilityThe button has a keyboard shortcut hint in the title attribute, but it might be beneficial to implement the actual keyboard shortcut handler using the
useKeyboardShortcuthook as seen in the LiveSwitchButton component.import { Calendar } from "@unkey/icons"; import { Button } from "@unkey/ui"; + import { useKeyboardShortcut } from "@/hooks/use-keyboard-shortcut"; import { useEffect, useState } from "react"; import { useFilters } from "../../../hooks/use-filters"; export const LogsDateTime = () => { const [title, setTitle] = useState<string | null>(null); const { filters, updateFilters } = useFilters(); + + // Add keyboard shortcut for toggling the datetime filter + useKeyboardShortcut({ key: "t" }, () => { + // Implementation to toggle the datetime popover + }); // Rest of the componentapps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx (1)
80-82: Consider enhancing keyboard shortcut accessibilityThe button has a keyboard shortcut hint in the title attribute, but it might be beneficial to implement the actual keyboard shortcut handler using the
useKeyboardShortcuthook as seen in other components like LiveSwitchButton.import { DatetimePopover } from "@/components/logs/datetime/datetime-popover"; import { cn } from "@/lib/utils"; import { Calendar } from "@unkey/icons"; import { Button } from "@unkey/ui"; + import { useKeyboardShortcut } from "@/hooks/use-keyboard-shortcut"; import { useEffect, useState } from "react"; import { useFilters } from "../../../../hooks/use-filters"; export const LogsDateTime = () => { const [title, setTitle] = useState<string | null>(null); const { filters, updateFilters } = useFilters(); + + // Add keyboard shortcut for toggling the datetime filter + useKeyboardShortcut({ key: "t" }, () => { + // Implementation to toggle the datetime popover + }); // Rest of the component
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/apis/_components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-display/index.tsx(1 hunks)apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/_components/controls/components/logs-datetime/index.tsx(1 hunks)apps/dashboard/components/logs/control-cloud/control-pill.tsx(1 hunks)apps/dashboard/components/logs/live-switch-button/index.tsx(1 hunks)apps/dashboard/components/logs/refresh-button/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/app/(app)/audit/components/controls/components/logs-datetime/index.tsx
- apps/dashboard/app/(app)/apis/_components/controls/components/logs-datetime/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/components/logs/refresh-button/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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: Test API / API Test Local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/controls/components/logs-datetime/index.tsx (1)
70-77: Button styling standardization looks good.The addition of
size="md"and therounded-lgclass to the button styling is consistent with the button component standardization in this PR.apps/dashboard/components/logs/control-cloud/control-pill.tsx (1)
74-87: Enhanced interactive states improve usability.The addition of
hover:bg-gray-4andfocus:hover:bg-gray-4classes improve the button's interactive feedback, providing better visual cues for users.These hover and focus states will make the control pill's close button more intuitive to interact with, especially for keyboard users navigating through the UI.
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/controls/components/logs-datetime/index.tsx (1)
70-77: Button styling update is consistent with design system changes.The addition of
size="md"property androunded-lgclass aligns with the button-v2 feature implementation across the application.apps/dashboard/app/(app)/logs/components/controls/components/logs-display/index.tsx (1)
9-14: Consistent button styling implementation.The addition of
size="md"and therounded-lgclass maintains visual consistency with other button component updates in this PR.apps/dashboard/components/logs/live-switch-button/index.tsx (1)
18-18: Consistent button styling appliedThe button has been updated with
size="md"androunded-lgclass to align with the new button standard being applied across the application.Also applies to: 21-21
apps/dashboard/app/(app)/logs/components/controls/components/logs-filters/index.tsx (1)
38-38: Consistent button styling appliedThe button has been updated with
size="md"androunded-lgclass, maintaining a consistent appearance with other buttons across the application and properly preserving conditional styling based on filter state.Also applies to: 40-41
apps/dashboard/app/(app)/ratelimits/_components/controls/components/logs-datetime/index.tsx (1)
72-72: Consistent button styling appliedThe button has been updated with
size="md"androunded-lgclass while properly preserving the conditional styling based on title state.Also applies to: 74-77
apps/dashboard/app/(app)/logs/components/controls/components/logs-datetime/index.tsx (1)
72-72: Consistent button styling appliedThe button has been updated with
size="md"androunded-lgclass while properly preserving the conditional styling based on title state.Also applies to: 74-77
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/engineering/content/design/components/button.mdx (2)
17-19: Minor grammatical improvement neededThere's a slight grammatical issue in this sentence.
- Button comes in three basic variants that serve different UI purposes: + The Button comes in three basic variants that serve different UI purposes:🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Possible missing article found.
Context: ...board interactions. ## Basic Variants Button comes in three basic variants that serv...(AI_HYDRA_LEO_MISSING_THE)
844-875: Comprehensive usage guidelinesThe usage guidelines section covers important considerations for button usage, including hierarchy, color semantics, interaction states, sizing, content guidelines, and accessibility.
One minor improvement suggestion for line 875:
- Consider stacking buttons vertically on very small screens + Consider stacking buttons vertically on compact or mobile screens🧰 Tools
🪛 LanguageTool
[style] ~875-~875: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/app/(app)/apis/_components/api-list-client.tsx(2 hunks)apps/dashboard/app/(app)/logs/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx(1 hunks)apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx(1 hunks)apps/engineering/content/design/components/button.mdx(1 hunks)apps/engineering/content/design/components/button.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/dashboard/app/(app)/ratelimits/[namespaceId]/logs/components/table/logs-table.tsx
- apps/dashboard/app/(app)/apis/_components/api-list-client.tsx
🧰 Additional context used
🪛 LanguageTool
apps/engineering/content/design/components/button.mdx
[uncategorized] ~18-~18: Possible missing article found.
Context: ...board interactions. ## Basic Variants Button comes in three basic variants that serv...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~875-~875: 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 (17)
- 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: Test Go API Local / test_agent_local
- GitHub Check: Build / Build
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (1)
208-208: Button component updated with appropriate size property.The addition of the
size="md"prop to the Button component aligns with the broader changes introduced in this PR's Button v2 feature. This ensures consistent button sizing across the application.apps/engineering/content/design/components/button.tsx (1)
6-41: Well-implemented keyboard shortcut component with good user feedback!The
ButtonWithKeyboardShortcutcomponent demonstrates a clean implementation of a button with keyboard shortcut capabilities. The code handles loading states correctly and provides clear visual feedback about the shortcut.Some minor enhancement suggestions:
- Consider adding a focus trap or ensuring the keyboard event only triggers when appropriate (e.g., when the component is visible/mounted)
- The simulated delay with
setTimeoutworks for demo purposes, but in a production environment, you might want to add error handling for async operationsconst handleClick = async () => { setIsLoading(true); - await new Promise((resolve) => setTimeout(resolve, 1000)); - setCount((prev) => prev + 1); - setIsLoading(false); + try { + await new Promise((resolve) => setTimeout(resolve, 1000)); + setCount((prev) => prev + 1); + } catch (error) { + console.error("Error during operation:", error); + } finally { + setIsLoading(false); + } };apps/engineering/content/design/components/button.mdx (9)
1-15: Comprehensive button documentation with clear descriptionThe metadata and introduction provide an excellent overview of the button component's capabilities, variants, and design considerations.
21-49: Well-structured primary button documentation with comprehensive state examplesThe primary button section provides clear examples of all important states (default, hover, focus, loading, disabled) in both light and dark themes.
51-153: Thorough documentation of outline and ghost variantsThe documentation for the outline and ghost variants includes all necessary states and clearly explains their use cases.
155-695: Comprehensive color variant documentationThe color variants (danger, warning, success) documentation is very thorough, covering all combinations of variants and states. The explanations for when to use each variant are clear and helpful.
697-725: Clear size variants demonstrationThe size variants section effectively demonstrates the range of available button sizes.
733-775: Well-documented icon supportThe documentation clearly demonstrates how to use icons within buttons, showing both leading and trailing icon placements.
777-781: Effective keyboard shortcut demonstrationThe keyboard shortcut section uses the custom component to demonstrate the feature, providing both visual cues and functional example.
789-831: Helpful additional features documentationThe legacy variant support and shape variants sections provide important information for backward compatibility and special layout cases.
835-842: Props documentation with auto-generated tableUsing the
AutoTypeTablecomponent to automatically generate the props documentation from TypeScript types is an excellent practice that ensures the documentation stays up-to-date with the code.apps/dashboard/app/(app)/logs/components/table/logs-table.tsx (1)
262-262:Details
✅ Verification successful
Button component updated with size prop
The addition of
size="md"to the Button component is part of the new button version implementation. This ensures consistent sizing across the application's UI.I noticed that in other files, buttons are being updated with both
size="md"andclassName="rounded-lg"according to the AI summary. Let's verify if this button should also have the rounded corners class for consistency:
🏁 Script executed:
#!/bin/bash # Description: Check for Button components with size="md" and see if they commonly have rounded-lg class echo "Searching for Button components with size prop..." rg "Button size=" --type=tsx -A 1 -B 1 echo "Checking if rounded-lg is commonly used with these buttons..." rg "Button size=\"md\"" --type=tsx -A 5 -B 5 | rg "rounded-lg"Length of output: 399
Action Needed: Manual Verification of Rounded Corners on Button Components
It looks like our initial automated search couldn’t reliably detect whether other Button instances add
className="rounded-lg"alongsidesize="md"—the TSX file type wasn’t recognized. To investigate further, please run the following revised shell script that limits the search to TSX files:If the output confirms that other Button components consistently include
className="rounded-lg", then the Button inapps/dashboard/app/(app)/logs/components/table/logs-table.tsxmight also need that class for consistency. Otherwise, if there’s no clear pattern, you should manually verify whether the current styling meets the design requirements.
🏁 Script executed:
#!/bin/bash echo "Searching for Button components with size prop in TSX files..." rg "Button size=" -g "*.tsx" -A 1 -B 1 echo "Searching for Button components with size=\"md\" and rounded-lg class in TSX files..." rg "Button size=\"md\"" -g "*.tsx" -A 5 -B 5 | grep "rounded-lg"Length of output: 3568
Button Component Update Verified
The updated Button with
size="md"is consistent with our current usage. Our search confirms that across the codebase, Button components using thesize="md"prop do not include aclassName="rounded-lg". Therefore, no additional changes are necessary for consistency at this time.
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Introduced a new button featuring a built-in keyboard shortcut for quick search.
Launched a unified dialog container for consistent modal interactions.
Expanded the icon set to enrich visual elements.
Refactor & Improvements
Revamped the button component with additional color variants, improved loading states, and enhanced accessibility.
Updated the UI color palette with new dynamic tokens and refined opacity settings.
Standardized modals, forms, and data displays for a smoother user experience.
Streamlined dialog implementations across multiple components.
Enhanced button styling with new size and rounded corner properties.
Documentation
Revised guidelines now include detailed examples on button usage and color schemes.
Enhanced documentation for color categories and accessibility features.
Bug Fixes
Improved visual feedback for buttons on hover and focus states.
Addressed inconsistencies in button sizes and styles across various components.
Updated button properties to ensure consistent appearance and usability.