Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@react-spectrum/s2/src/ComboBox.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ const ComboboxInner = forwardRef(function ComboboxInner(props: ComboBoxProps<any
}],
[TextContext, {
slots: {
'description': {styles: description({size})}
'description': {styles: description({size, isFocused: false, isDisabled: false})}
}
}]
]}>
Expand Down
62 changes: 51 additions & 11 deletions packages/@react-spectrum/s2/src/ListView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,15 @@ const listitem = style<GridListItemRenderProps & {
color: {
default: baseColor('neutral-subdued'),
isSelected: baseColor('neutral'),
isDisabled: {
default: 'disabled',
forcedColors: 'GrayText'
isDisabled: 'disabled',
forcedColors: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the description text styles match the label text styles?

Image

default: 'ButtonText',
selectionStyle: {
highlight: {
isSelected: 'HighlightText'
}
},
isDisabled: 'GrayText'
}
},
position: 'relative',
Expand Down Expand Up @@ -346,12 +352,14 @@ const listitem = style<GridListItemRenderProps & {
borderColor: {
default: '--borderColor',
isNextSelected: 'transparent',
isSelected: 'transparent'
isSelected: 'transparent',
forcedColors: 'ButtonBorder'
},
'--radius': {
type: 'borderTopStartRadius',
value: 'default'
}
},
forcedColorAdjust: 'none'
});

const insetBorderRadius = 'calc(var(--radius) - 1px)';
Expand All @@ -373,7 +381,8 @@ const listRowBackground = style<GridListItemRenderProps & {
isSelected: '[-1px]',
// Don't overlap focus ring of row above.
isPrevSelected: 0,
isFirstItem: 0
isFirstItem: 0,
forcedColors: 0
},
left: 0,
right: 0,
Expand Down Expand Up @@ -412,7 +421,12 @@ const listRowBackground = style<GridListItemRenderProps & {
}
},
forcedColors: {
default: 'Background'
default: 'Background',
selectionStyle: {
highlight: {
isSelected: 'Highlight'
}
}
}
},
borderTopStartRadius: {
Expand Down Expand Up @@ -513,9 +527,34 @@ const listRowBackground = style<GridListItemRenderProps & {
}
});

let listRowFocusRing = style({
let listRowFocusRing = style<GridListItemRenderProps & {
selectionStyle?: 'highlight' | 'checkbox',
isFirstItem?: boolean,
isPrevSelected?: boolean,
isPrevNotSelected?: boolean,
isNextSelected?: boolean,
isNextNotSelected?: boolean,
isLastItem?: boolean,
isQuiet?: boolean
}>({
...focusRing(),
outlineOffset: -2,
outlineOffset: {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we like this "style" in HCM? or is it not clear enough?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's fine for now? it's def better than using the normal focus ring hcm and i can tell that it's a little thicker

default: -2,
forcedColors: -3
},
outlineWidth: {
default: 2,
forcedColors: '[3px]'
},
outlineColor: {
default: 'focus-ring',
forcedColors: {
default: 'Highlight',
selectionStyle: {
highlight: 'ButtonBorder'
}
}
},
position: 'absolute',
inset: 0,
top: {
Expand Down Expand Up @@ -567,7 +606,8 @@ export let description = style({
font: 'ui-sm',
color: {
default: baseColor('neutral-subdued'),
isDisabled: 'disabled'
isDisabled: 'disabled',
forcedColors: 'inherit'
},
transition: 'default'
});
Expand Down Expand Up @@ -770,7 +810,7 @@ export function ListViewItem(props: ListViewItemProps): ReactNode {
isLastItem: isLastItem(id, state)
})
} />
{renderProps.isFocusVisible &&
{renderProps.isFocusVisible &&
<div
className={listRowFocusRing({
...renderProps,
Expand Down
26 changes: 14 additions & 12 deletions packages/@react-spectrum/s2/src/Menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,11 @@ export let menuitem = style<Omit<MenuItemRenderProps, 'hasSubmenu' | 'isOpen'> &
},
color: {
default: baseColor('neutral'),
isDisabled: 'disabled',
forcedColors: {
default: 'ButtonText',
isFocused: 'HighlightText'
},
isDisabled: {
default: 'disabled',
forcedColors: 'GrayText'
isFocused: 'HighlightText',
isDisabled: 'GrayText'
}
},
position: 'relative',
Expand Down Expand Up @@ -278,7 +276,7 @@ export let label = style<{size: string}>({
marginTop: '--labelPadding'
});

export let description = style({
export let description = style<{size: 'S' | 'M' | 'L' | 'XL', isFocused: boolean, isDisabled: boolean}>({
gridArea: 'description',
font: {
default: 'ui-sm',
Expand All @@ -294,7 +292,10 @@ export let description = style({
// Ideally this would use the same token as hover, but we don't have access to that here.
// TODO: should we always consider isHovered and isFocused to be the same thing?
isFocused: 'gray-800',
isDisabled: 'disabled'
isDisabled: 'disabled',
forcedColors: {
default: 'inherit'
}
},
transition: 'default'
});
Expand All @@ -304,7 +305,7 @@ let value = style({
marginStart: 8
});

let keyboard = style<{size: 'S' | 'M' | 'L' | 'XL', isDisabled: boolean}>({
let keyboard = style<{size: 'S' | 'M' | 'L' | 'XL', isDisabled: boolean, isFocused: boolean}>({
gridArea: 'keyboard',
marginStart: 8,
font: 'ui',
Expand All @@ -313,7 +314,7 @@ let keyboard = style<{size: 'S' | 'M' | 'L' | 'XL', isDisabled: boolean}>({
default: 'gray-600',
isDisabled: 'disabled',
forcedColors: {
isDisabled: 'GrayText'
default: 'inherit'
}
},
unicodeBidi: 'plaintext'
Expand Down Expand Up @@ -386,7 +387,7 @@ export const Menu = /*#__PURE__*/ (forwardRef as forwardRefType)(function Menu<T
}],
[TextContext, {
slots: {
'description': {styles: description({size})}
'description': {styles: description({size, isFocused: false, isDisabled: false})}
}
}],
[InPopoverContext, false]
Expand Down Expand Up @@ -526,6 +527,7 @@ export function MenuItem(props: MenuItemProps): ReactNode {
{(renderProps) => {
let {children} = props;
let checkboxRenderProps = {...renderProps, size, isFocused: false, isFocusVisible: false, isIndeterminate: false, isReadOnly: false, isInvalid: false, isRequired: false};
let isFocused = (renderProps.hasSubmenu && renderProps.isOpen) || renderProps.isFocused;
return (
<>
<Provider
Expand All @@ -540,11 +542,11 @@ export function MenuItem(props: MenuItemProps): ReactNode {
slots: {
[DEFAULT_SLOT]: {styles: label({size})},
label: {styles: label({size})},
description: {styles: description({...renderProps, size})},
description: {styles: description({...renderProps, size, isFocused})},
value: {styles: value}
}
}],
[KeyboardContext, {styles: keyboard({size, isDisabled: renderProps.isDisabled})}],
[KeyboardContext, {styles: keyboard({...renderProps, size, isFocused})}],
[ImageContext, {styles: image({size})}]
]}>
{renderProps.selectionMode === 'single' && !renderProps.hasSubmenu && <CheckmarkIcon size={checkmarkIconSize[size]} className={checkmark({...renderProps, size})} />}
Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/s2/src/Picker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,7 @@ export const Picker = /*#__PURE__*/ (forwardRef as forwardRefType)(function Pick
}],
[TextContext, {
slots: {
description: {styles: description({size})}
'description': {styles: description({size, isFocused: false, isDisabled: false})}
}
}]
]}>
Expand Down
5 changes: 4 additions & 1 deletion packages/@react-spectrum/s2/src/TableView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,10 @@ const cellFocus = {
},
outlineOffset: -2,
outlineWidth: 2,
outlineColor: 'focus-ring',
outlineColor: {
default: 'focus-ring',
forcedColors: 'Highlight'
},
borderRadius: '[6px]'
} as const;

Expand Down
2 changes: 1 addition & 1 deletion packages/@react-spectrum/s2/src/TabsPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ function Picker<T extends object>(props: PickerProps<T>, ref: FocusableRef<HTMLB
}],
[TextContext, {
slots: {
description: {styles: description({size})}
'description': {styles: description({size, isFocused: false, isDisabled: false})}
}
}]
]}>
Expand Down
3 changes: 2 additions & 1 deletion packages/@react-spectrum/s2/stories/ListView.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ const meta: Meta<typeof ListView> = {
},
tags: ['autodocs'],
argTypes: {
...categorizeArgTypes('Events', ['onSelectionChange'])
...categorizeArgTypes('Events', ['onSelectionChange']),
children: {table: {disable: true}}
},
title: 'ListView',
args: {
Expand Down
17 changes: 17 additions & 0 deletions packages/@react-spectrum/s2/style/__tests__/style-macro.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,23 @@ describe('style-macro', () => {
expect(js({isSelected: true})).toMatchInlineSnapshot('" ple12 -macro-dynamic-37zkvn"');
});

it('inherits parent default when nested branch has no default key', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do people feel about changing the style macro to support inheriting the nearest default? I think it makes more sense and I've continually forgotten about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i've run into this too

let {css, js} = testStyle({
color: {
forcedColors: {
default: 'ButtonText',
variant: {
highlight: {isSelected: 'HighlightText'}
}
}
}
});
// forcedColors.default should apply when variant=highlight but !isSelected
expect(css).toContain('ButtonText');
expect(js({variant: 'highlight'})).toMatchInlineSnapshot('" plb12 -macro-dynamic-1owjb9s"');
expect(js({variant: 'highlight', isSelected: true})).toMatchInlineSnapshot('" ple12 -macro-dynamic-37zkvn"');
});

it('should expand shorthand properties to longhands', () => {
let {js, css} = testStyle({
padding: 24
Expand Down
15 changes: 11 additions & 4 deletions packages/@react-spectrum/s2/style/style-macro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ export function createTheme<T extends Theme>(theme: T): StyleFunction<ThemePrope
// We also check if this is globalThis, which happens in non-strict mode bundles.
// Also allow style to be called as a normal function in tests.
// @ts-ignore

if ((this == null || this === globalThis) && process.env.NODE_ENV !== 'test') {
throw new Error('The style macro must be imported with {type: "macro"}.');
}
Expand Down Expand Up @@ -478,7 +478,14 @@ export function createTheme<T extends Theme>(theme: T): StyleFunction<ThemePrope
priority = Math.max(priority, subPriority);
} else if (val && typeof val === 'object' && !Array.isArray(val)) {
for (let key in val) {
let [subPriority, subRules] = conditionalToRules(val[key], rulePriority, currentConditions, subSkipConditions, fn);
let branchValue = val[key];
// If this branch has no default, inherit the parent's default so e.g. forcedColors.default
// applies when selectionStyle.highlight doesn't define its own default.
// eslint-disable-next-line max-depth
if (value.default !== undefined && branchValue && typeof branchValue === 'object' && !Array.isArray(branchValue) && !('default' in branchValue)) {
branchValue = {default: value.default, ...branchValue};
}
let [subPriority, subRules] = conditionalToRules(branchValue, rulePriority, currentConditions, subSkipConditions, fn);
rules.push(...compileCondition(currentConditions, `${condition} === ${JSON.stringify(key)}`, priority, subRules));
priority = Math.max(priority, subPriority);
}
Expand Down Expand Up @@ -868,7 +875,7 @@ export function raw(this: MacroContext | void, css: string, layer = '_.a'): stri
// We also check if this is globalThis, which happens in non-strict mode bundles.
// Also allow style to be called as a normal function in tests.
// @ts-ignore

if ((this == null || this === globalThis) && process.env.NODE_ENV !== 'test') {
throw new Error('The raw macro must be imported with {type: "macro"}.');
}
Expand Down Expand Up @@ -898,7 +905,7 @@ export function keyframes(this: MacroContext | void, css: string): string {
// We also check if this is globalThis, which happens in non-strict mode bundles.
// Also allow style to be called as a normal function in tests.
// @ts-ignore

if ((this == null || this === globalThis) && process.env.NODE_ENV !== 'test') {
throw new Error('The keyframes macro must be imported with {type: "macro"}.');
}
Expand Down