-
Notifications
You must be signed in to change notification settings - Fork 2.3k
refactor: Components updates #7308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 32 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="web/src/app/admin/configuration/llm/forms/components/FormWrapper.tsx">
<violation number="1" location="web/src/app/admin/configuration/llm/forms/components/FormWrapper.tsx:123">
P2: Removing `max-h-[70vh] overflow-y-auto` from a `medium` variant modal may cause content clipping. The Modal component only applies automatic overflow scrolling for `large`, `small`, and `tall` variants—not `medium`. If form content is tall, it could be clipped rather than scrollable. Consider keeping the scroll styles, or switching to a variant like `tall` that handles overflow automatically.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile OverviewGreptile SummaryThis PR refactors the Modal component system to standardize styling and remove manual className overrides across the codebase. Key ChangesModal Component Refactoring (
Section Component Enhancement (
Cleanup:
ImpactThis refactoring standardizes modal styling across the entire application, making modals more consistent and reducing boilerplate. The default Confidence Score: 5/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant Modal as Modal Component
participant Section as Section Layout
participant Body as Modal.Body
participant Footer as Modal.Footer
Note over Dev,Footer: PR refactors Modal component system
Dev->>Modal: Remove className prop support
Note over Modal: Use WithoutStyles type
Dev->>Section: Add forwardRef support
Dev->>Section: Add "between" to JustifyContent
Dev->>Modal: Refactor Modal.Header
Modal->>Section: Use Section instead of div
Dev->>Modal: Refactor Modal.Body
Modal->>Body: Add twoTone prop (default: true)
Modal->>Body: Add auto overflow for fixed-height variants
Body->>Section: Wrap Section in div with background
Dev->>Modal: Refactor Modal.Footer
Modal->>Footer: Replace div with Section
Modal->>Footer: Set default layout (row, end, gap 0.5)
Dev->>Modal: Add sizeVariant to context
Note over Body: Use sizeVariant for overflow logic
Dev->>Dev: Update all Modal usages (32 files)
Note over Dev: Remove manual className styling
|
|
|
||
| return ( | ||
| <div className="relative"> | ||
| <div className="relative h-full"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an example would be helpful, but this seems wrong as a default? Feel like ShadowDiv should behave like a div.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe rename the component to make it clear this has default layout properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe @nmgarza5 can update the name? He was the one who originally named it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this change load-bearing? Can we follow up with this change? It's a functional change and realistically does not belong in a refactor. I also think it's incorrect, so we should resolve this before merge imo.
Subash-Mohan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
jmelahman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go over this tomorrow?
|
Actually, @jmelahman, I can just revert the changes to |
Description
Summary by cubic
Standardized the Modal API and styling across the app and aligned shared components for consistent spacing and scroll behavior. Removed ad‑hoc classNames in consumers and simplified the UserFilesModal search experience.
Refactors
Migration
Written for commit 9604076. Summary will update on new commits.