-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: Section layout component fix
#7305
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.
No issues found across 1 file
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.
Greptile Overview
Greptile Summary
This PR adds alignItems="start" to the GeneralLayouts.Section component inside Card to fix layout issues where card content was being centered instead of aligned to the start.
Analysis
The change addresses a layout inconsistency where cards were centering their content due to the Section component's default alignItems="center". By explicitly setting alignItems="start", card content will now align to the start as intended.
However, there's a critical prop ordering issue: the alignItems="start" is placed before the {...props} spread. Since CardProps extends GeneralLayouts.SectionProps, users can pass alignItems through props, which would override the hardcoded value, defeating the purpose of this fix.
Impact
- Breaking behavior: If any code currently passes
alignItemstoCard(none found in current search), the fix won't work for those instances - Intended fix won't be enforceable: The layout issue can reappear if
alignItemsis passed through props
Confidence Score: 3/5
- Safe to merge with one prop ordering issue that reduces fix effectiveness
- The change is functionally correct and won't break existing code since no current usages pass alignItems to Card. However, the prop ordering issue means the fix can be accidentally overridden, reducing its effectiveness. This is a logic issue that should be addressed to ensure the fix works as intended in all cases.
- web/src/refresh-components/cards/Card.tsx needs prop reordering to ensure alignItems="start" cannot be overridden
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| web/src/refresh-components/cards/Card.tsx | 3/5 | Added alignItems="start" to Section component, but placement before {...props} allows override, defeating the fix's purpose |
Sequence Diagram
sequenceDiagram
participant User
participant Card
participant Section
User->>Card: Render with props
Note over Card: Extract translucent, disabled, padding
Card->>Card: Determine variant (main/translucent/disabled)
Card->>Card: Create wrapper div with styles
Note over Card: Pass alignItems="start" first
Note over Card: Then spread {...props}
Note over Card: If props.alignItems exists, it overrides!
Card->>Section: Render with padding, alignItems, and ...props
Section->>Section: Apply flexbox layout
Section-->>Card: Rendered Section
Card-->>User: Rendered Card with Section inside
| return ( | ||
| <div className={cn("rounded-16 w-full h-full", classNames[variant])}> | ||
| <GeneralLayouts.Section padding={padding} {...props} /> | ||
| <GeneralLayouts.Section alignItems="start" padding={padding} {...props} /> |
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.
The alignItems="start" prop is placed before {...props}, which means any alignItems value passed through props will override it. Since CardProps extends GeneralLayouts.SectionProps, users can pass alignItems and it will defeat this fix. Move alignItems="start" after {...props} to ensure it cannot be overridden:
| <GeneralLayouts.Section alignItems="start" padding={padding} {...props} /> | |
| <GeneralLayouts.Section padding={padding} {...props} alignItems="start" /> |
Description
This PR makes
Sectionalign to the start.Summary by cubic
Set GeneralLayouts.Section alignItems="start" inside Card so card content aligns to the start. Fixes cards that were centering content and causing layout inconsistencies.
Written for commit c3166fa. Summary will update on new commits.