Skip to content

Commit 5f98980

Browse files
committed
feat(many): add margin prop
1 parent 7450ddc commit 5f98980

File tree

20 files changed

+47660
-18075
lines changed

20 files changed

+47660
-18075
lines changed

CHECKBOX_MARGIN_SOLUTION.md

Lines changed: 232 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,232 @@
1+
# Checkbox Margin Prop - Solution Using mapSpacingToShorthand
2+
3+
## Problem
4+
5+
The Checkbox component's `margin` prop worked with theme values like `"large"` but NOT with custom CSS values like `"40px"`.
6+
7+
## Root Cause
8+
9+
The Checkbox was using the `View` component wrapper, which calls `getShorthandPropValue()` without the `matchCSSUnits` parameter. This function requires an explicit flag to accept custom CSS units.
10+
11+
## Why CheckboxGroup Worked
12+
13+
CheckboxGroup → FormFieldGroup → FormFieldLayout uses `mapSpacingToShorthand()` instead, which is more permissive:
14+
15+
```typescript
16+
// mapSpacingToShorthand - more permissive
17+
splitMargin.map((m: string) => spacingMap[m] || m)
18+
// ^^^^^^^^^^^^^^
19+
// Returns original value if not found in theme!
20+
```
21+
22+
## Solution
23+
24+
Instead of modifying the View component (which would affect the entire system), we:
25+
26+
1. **Removed the View wrapper** from Checkbox
27+
2. **Used `mapSpacingToShorthand` directly** in the render method
28+
3. **Applied margin as inline style** to the root div
29+
30+
## Implementation
31+
32+
### Files Changed
33+
34+
#### 1. packages/ui-checkbox/src/Checkbox/index.tsx
35+
36+
**Import added:**
37+
38+
```typescript
39+
import { withStyle, mapSpacingToShorthand } from '@instructure/emotion'
40+
```
41+
42+
**Render method updated:**
43+
44+
```typescript
45+
render() {
46+
const { margin, /* ...other props */ } = this.props
47+
48+
// Calculate margin using mapSpacingToShorthand
49+
const cssMargin = margin
50+
? mapSpacingToShorthand(margin, this.theme?.spacing || {})
51+
: undefined
52+
53+
return (
54+
<div
55+
css={styles?.checkbox}
56+
style={{ margin: cssMargin }} // Apply inline
57+
// ... rest of props
58+
>
59+
{/* children */}
60+
</div>
61+
)
62+
}
63+
```
64+
65+
#### 2. packages/ui-checkbox/src/Checkbox/styles.ts
66+
67+
**Import added:**
68+
69+
```typescript
70+
import { mapSpacingToShorthand } from '@instructure/emotion'
71+
```
72+
73+
**generateStyle updated:**
74+
75+
```typescript
76+
const generateStyle = (
77+
componentTheme: ComponentTheme,
78+
props: CheckboxProps
79+
): CheckboxStyle => {
80+
const { inline, disabled, margin } = props // Added margin
81+
// ... rest of function
82+
}
83+
```
84+
85+
#### 3. packages/ui-view/src/View/styles.ts
86+
87+
**Reverted changes** - removed `matchCSSUnits: true` parameters to keep View unchanged.
88+
89+
## How It Works
90+
91+
### With Theme Values
92+
93+
```tsx
94+
<Checkbox label="Test" value="1" margin="large" />
95+
```
96+
97+
1. `mapSpacingToShorthand("large", theme.spacing)`
98+
2. Looks up `large` in `theme.spacing`
99+
3. Finds `"1.5rem"` (or whatever the theme defines)
100+
4. Returns `"1.5rem"`
101+
5. Applied as `style={{ margin: "1.5rem" }}`
102+
103+
### With Custom CSS Values
104+
105+
```tsx
106+
<Checkbox label="Test" value="1" margin="40px" />
107+
```
108+
109+
1. `mapSpacingToShorthand("40px", theme.spacing)`
110+
2. Looks up `40px` in `theme.spacing`
111+
3. **NOT FOUND** - returns original value `"40px"`
112+
4. Returns `"40px"`
113+
5. Applied as `style={{ margin: "40px" }}`
114+
115+
### With Multiple Values
116+
117+
```tsx
118+
<Checkbox label="Test" value="1" margin="large 40px" />
119+
```
120+
121+
1. Splits into `["large", "40px"]`
122+
2. Maps each:
123+
- `"large"` → theme value → `"1.5rem"`
124+
- `"40px"` → not in theme → `"40px"`
125+
3. Joins back → `"1.5rem 40px"`
126+
4. Applied as `style={{ margin: "1.5rem 40px" }}`
127+
128+
## Testing
129+
130+
```bash
131+
# Rebuild the package
132+
npm run build
133+
134+
# Or watch mode
135+
npm run build:watch
136+
```
137+
138+
### Test Cases
139+
140+
```tsx
141+
import { Checkbox } from '@instructure/ui-checkbox'
142+
143+
function TestCheckboxMargin() {
144+
return (
145+
<div style={{ border: '1px solid red', padding: '20px' }}>
146+
{/* Theme value */}
147+
<Checkbox label="margin='large'" value="1" margin="large" />
148+
149+
{/* Custom pixel value */}
150+
<Checkbox label="margin='40px'" value="2" margin="40px" />
151+
152+
{/* Custom rem value */}
153+
<Checkbox label="margin='2rem'" value="3" margin="2rem" />
154+
155+
{/* Mixed values */}
156+
<Checkbox label="margin='large 40px'" value="4" margin="large 40px" />
157+
158+
{/* Shorthand */}
159+
<Checkbox label="margin='10px 20px'" value="5" margin="10px 20px" />
160+
161+
{/* All four sides */}
162+
<Checkbox
163+
label="margin='5px 10px 15px 20px'"
164+
value="6"
165+
margin="5px 10px 15px 20px"
166+
/>
167+
</div>
168+
)
169+
}
170+
```
171+
172+
## Advantages of This Approach
173+
174+
**No system-wide changes** - View component remains unchanged
175+
**Consistent with FormFieldLayout** - Uses same `mapSpacingToShorthand` function
176+
**Supports both theme and custom values** - Automatic fallback behavior
177+
**Simple implementation** - Just inline styles, no complex wrapper logic
178+
**Type-safe** - Uses existing `Spacing` type
179+
180+
## Apply to Other Components
181+
182+
This same pattern should be applied to:
183+
184+
- RadioInput
185+
- RangeInput
186+
- Text (if not already using View properly)
187+
- ToggleButton
188+
- Any other individual form controls
189+
190+
### Template for Other Components:
191+
192+
```typescript
193+
// 1. Import mapSpacingToShorthand
194+
import { mapSpacingToShorthand } from '@instructure/emotion'
195+
196+
// 2. In render method
197+
render() {
198+
const { margin } = this.props
199+
200+
const cssMargin = margin
201+
? mapSpacingToShorthand(margin, this.theme?.spacing || {})
202+
: undefined
203+
204+
return (
205+
<div
206+
css={styles?.rootElement}
207+
style={{ margin: cssMargin }}
208+
// ... rest of props
209+
>
210+
{/* children */}
211+
</div>
212+
)
213+
}
214+
```
215+
216+
## Summary
217+
218+
**Before:**
219+
220+
- Checkbox wrapped with View → used `getShorthandPropValue` → required `matchCSSUnits: true` → didn't work with custom values
221+
222+
**After:**
223+
224+
- Checkbox uses `mapSpacingToShorthand` directly → permissive fallback → works with both theme and custom values ✅
225+
226+
**Result:**
227+
228+
```tsx
229+
// Both work now!
230+
<Checkbox label="Theme" value="1" margin="large" /> // ✅ Works
231+
<Checkbox label="Custom" value="2" margin="40px" /> // ✅ Works
232+
```

0 commit comments

Comments
 (0)