feat: view #121
feat: view #121
Conversation
size-limit report 📦
|
src/view/View.tsx
Outdated
| borderRadius?: 'small' | 'medium'; | ||
| borderColor?: 'light' | 'dark'; | ||
| backgroundColor?: 'light' | 'dark'; |
There was a problem hiding this comment.
@JackyxCS we need to move these to explicit types that can be re-used like DimensionValue e.g. we need BorderRadius etc.
src/view/View.tsx
Outdated
| : 'transparent'}; | ||
| border-radius: ${borderRadius != null | ||
| ? `var(--ac-global-rounding-${borderRadius})` | ||
| : 0}; | ||
| width: ${width != null ? dimensionValue(width) : 0}; |
There was a problem hiding this comment.
@JackyxCS let's look into how spectrum gets a nice default width. I think that's probably the behavior we want
|
@mikeldking took another pass and added types and add'l gallery views, lmk if anything else to consider! Seems like a default of 'auto' for width and height matches spectrum's behavior |
src/types/core.ts
Outdated
|
|
||
| export type BorderColorValue = BorderColorAlias; | ||
|
|
||
| export type BackgroundColorValue = BorderColorAlias; |
There was a problem hiding this comment.
I wouldn't assign types like this where the semantics doesn't make sense. Naming things properly is very important - code is read 90% more than it is written. I would argue that you could just have assigned them to separate sets.
| export type BackgroundColorValue = BorderColorAlias; | |
| type BorderColorValue = "light" | "dark"; | |
| type BackgroundColorValue = "light" | dark"; |
This is more true to form - they legitimately are separate values
If the concept of light and dark is meant to be consistent across, then something along the lines of
type LightOrDark = "light" | "dark"Could be used.
There was a problem hiding this comment.
got it, missed the fact that light and dark are truly separate colors for border/background and will keep the naming in mind for the future!
src/view/View.tsx
Outdated
| height: ${height != null ? dimensionValue(height) : 'auto'}; | ||
| `} | ||
| className="ac-view" | ||
| aria-labelledby={viewId} |
There was a problem hiding this comment.
this seems incorrect - what's the rationale here?
There was a problem hiding this comment.
I think this should just be id={id} misinterpreted the use of aria-labelledby since there's not another element to relate it to
There was a problem hiding this comment.
guess we wouldn't need an aria-label here since it's not an interactive element?
There was a problem hiding this comment.
aria-labelledby is typically for form fields https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby
It's not needed if you don't use semantic markup. E.g.
<label><input /></label>doesn't need this vs
<label id="foo">foo</label>
<input aria-labelledby="foo" />
resolves #122