feat: style lint rules - gap, margin, padding#10013
Conversation
BF150
changed the title
|
🚀 MCP preview deployed to Vercel: https://kolibri-n6yvibkjf-public-ui-kolibri-mcp.vercel.app |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enforces stricter CSS linting rules, including mandatory CSS layers and restrictions on gap, margin, and padding properties. It also refactors select component styles and adjusts input container selectors. Feedback points out that using flexbox on native option elements is poorly supported and suggests longhand padding as an alternative. Additionally, reviewers expressed concerns about the total ban on gap properties, the bypassability of shorthand restrictions via longhand properties, and potential layout regressions caused by removing property resets in the default theme.
| display: flex; | ||
| align-items: center; | ||
| min-height: var(--a11y-min-size); |
There was a problem hiding this comment.
Styling native <option> elements with display: flex and min-height is generally not supported by major browsers (e.g., Chrome, Safari) as they ignore most layout properties on native options. This change will likely break the vertical alignment that the previous padding-based approach provided. To maintain compatibility while adhering to the new lint rule that restricts the padding shorthand, consider using longhand padding properties.
| display: flex; | |
| align-items: center; | |
| min-height: var(--a11y-min-size); | |
| padding-top: calc((var(--a11y-min-size) - to-rem(16)) / 2); | |
| padding-bottom: calc((var(--a11y-min-size) - to-rem(16)) / 2); | |
| padding-inline: 0.5em; |
| } | ||
| ] | ||
| ], | ||
| "property-disallowed-list": ["gap", "row-gap", "column-gap"], |
There was a problem hiding this comment.
Disallowing gap properties entirely prevents the use of modern flexbox and grid spacing techniques. gap is often preferred over margins for managing internal spacing between component parts (like icons and labels) because it avoids side effects on the container's edges. A blanket ban might lead to less maintainable code using child margins. Consider allowing gap while perhaps restricting its values to theme-approved tokens.
| "declaration-property-value-allowed-list": { | ||
| "margin": ["0", "auto"], | ||
| "padding": ["0"] | ||
| } |
There was a problem hiding this comment.
The declaration-property-value-allowed-list for margin and padding only restricts the shorthand properties. This policy is easily bypassed by using longhand properties like margin-top or padding-left. If the intent is to enforce a strict "no-internal-spacing" architecture for components, these longhand properties should also be added to the property-disallowed-list to ensure the rule is effective and consistent.
| position: relative; | ||
| padding: 0; | ||
| gap: 0; | ||
| grid-template-columns: auto max-content min-content; |
There was a problem hiding this comment.
The removal of padding: 0; and gap: 0; from .kol-input-container may cause layout regressions if these properties are inherited from base styles. Given that the Kern theme still explicitly sets these resets, this change introduces an inconsistency between themes that could lead to visual bugs in the default theme.
|
Netlify Draft Deployment |
1 participant
No description provided.