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
5 changes: 3 additions & 2 deletions packages/@react-spectrum/s2/src/ActionMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {useSpectrumContextProps} from './useSpectrumContextProps';

export interface ActionMenuProps<T> extends
Pick<MenuTriggerProps, 'isOpen' | 'defaultOpen' | 'onOpenChange' | 'align' | 'direction' | 'shouldFlip'>,
Pick<MenuProps<T>, 'children' | 'items' | 'disabledKeys' | 'onAction'>,
Pick<MenuProps<T>, 'children' | 'items' | 'disabledKeys' | 'onAction' | 'shouldCloseOnSelect'>,
Pick<ActionButtonProps, 'isDisabled' | 'isQuiet' | 'autoFocus' | 'size'>,
StyleProps, DOMProps, AriaLabelingProps {
/**
Expand Down Expand Up @@ -72,7 +72,8 @@ export const ActionMenu = /*#__PURE__*/(forwardRef as forwardRefType)(function A
items={props.items}
disabledKeys={props.disabledKeys}
onAction={props.onAction}
size={props.menuSize}>
size={props.menuSize}
shouldCloseOnSelect={props.shouldCloseOnSelect}>
{props.children}
</Menu>
</MenuTrigger>
Expand Down
41 changes: 34 additions & 7 deletions packages/@react-spectrum/s2/src/TableView.tsx
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Noticed the focus ring doesn't appear on the TableView's checkbox now. What is the root issue of the checkbox alignment issue?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Honestly, I'm not entirely sure, something about alignContent worked to centre it everywhere except for in Safari mobile. Mostly seemed like a happy accident since there was no display flex or anything on the containing cell

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok, it looks like it was previously depending on span being text aligned center, but safari ios just for some reason wouldn't do that. centering that way was always iffy, flex is better, more predictable

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

gotcha, thanks for the explanation. Unrelated, I was hoping that these changes would also fix the weird FF select all check box cell sizing as a side effect but no luck :(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yeah, i haven't had any thoughts on how to address that yet :-/

Original file line number Diff line number Diff line change
Expand Up @@ -867,16 +867,26 @@ const tableHeader = style({
});

const selectAllCheckbox = style({
marginStart: 16 // table-edge-to-content, same between mobile and desktop
});

const selectAllCheckboxColumn = style({
padding: 0,
paddingStart: {
default: 0,
':has([slot="selection"])': 16
},
paddingEnd: {
default: 0,
':has(slot="selection")': 8
},
paddingY: 0,
height: 'full',
boxSizing: 'border-box',
outlineStyle: 'none',
position: 'relative',
display: 'flex',
alignContent: 'center',
alignItems: 'center',
justifyContent: 'start',
borderColor: {
default: 'gray-300',
forcedColors: 'ButtonBorder'
Expand Down Expand Up @@ -1009,15 +1019,22 @@ const stickyCell = {
const checkboxCellStyle = style({
...commonCellStyles,
...stickyCell,
display: 'flex',
paddingStart: 16,
paddingEnd: 8,
alignContent: 'center',
alignItems: 'center',
justifyContent: 'start',
height: 'calc(100% - 1px)',
borderBottomWidth: 0,
backgroundColor: '--rowBackgroundColor'
});

const cellContent = style({
truncate: true,
truncate: {
default: true,
isSticky: false
},
whiteSpace: {
default: 'nowrap',
overflowMode: {
Expand All @@ -1031,7 +1048,10 @@ const cellContent = style({
end: 'end'
}
},
width: 'full',
width: {
default: 'full',
':has([slot="selection"])': 'unset'
},
isolation: 'isolate',
padding: {
default: 4,
Expand All @@ -1058,7 +1078,14 @@ export interface CellProps extends Omit<RACCellProps, 'style' | 'className' | 'r
* A cell within a table row.
*/
export const Cell = forwardRef(function Cell(props: CellProps, ref: DOMRef<HTMLDivElement>) {
let {children, isSticky, showDivider = false, align, textValue, ...otherProps} = props;
let {
children,
isSticky,
showDivider = false,
align,
textValue,
...otherProps
} = props;
let domRef = useDOMRef(ref);
let tableVisualOptions = useContext(InternalTableContext);
textValue ||= typeof children === 'string' ? children : undefined;
Expand All @@ -1079,7 +1106,7 @@ export const Cell = forwardRef(function Cell(props: CellProps, ref: DOMRef<HTMLD
{...otherProps}>
{({id, isFocusVisible, hasChildItems, isTreeColumn, isExpanded, isDisabled}) => (
<>
{hasChildItems && isTreeColumn &&
{hasChildItems && isTreeColumn &&
<ExpandableRowChevron key={id} isDisabled={isDisabled} isExpanded={isExpanded} />
}
<span className={cellContent({...tableVisualOptions, isSticky, align: align || 'start'})}>{children}</span>
Expand Down Expand Up @@ -1252,7 +1279,7 @@ export const EditableCell = forwardRef(function EditableCell(props: EditableCell
{...otherProps}>
{({id, isFocusVisible, hasChildItems, isTreeColumn, isExpanded, isDisabled}) => (
<>
{hasChildItems && isTreeColumn &&
{hasChildItems && isTreeColumn &&
<ExpandableRowChevron key={id} isDisabled={isDisabled} isExpanded={isExpanded} />
}
<EditableCellInner {...props} isFocusVisible={isFocusVisible} cellRef={domRef as RefObject<HTMLDivElement>} />
Expand Down
11 changes: 5 additions & 6 deletions packages/@react-spectrum/s2/style/spectrum-theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1112,12 +1112,11 @@ export const style = createTheme({
animationDuration: 150,
animationTimingFunction: 'default'
}),
// eslint-disable-next-line @typescript-eslint/no-unused-vars
truncate: (_value: true) => ({
overflowX: 'hidden',
overflowY: 'hidden',
textOverflow: 'ellipsis',
whiteSpace: 'nowrap'
truncate: (value: boolean) => ({
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure why i can't set these to undefined, or return an empty object

overflowX: value ? 'hidden' : 'visible',
overflowY: value ? 'hidden' : 'visible',
textOverflow: value ? 'ellipsis' : 'clip',
whiteSpace: value ? 'nowrap' : 'normal'
}),
font: (value: keyof typeof fontSize) => {
let type = value.split('-')[0];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,18 @@ let actionMenuItems = [
</div>"
`;

exports[`Renames closeOnSelect to shouldCloseOnSelect 1`] = `
"import { MenuItem, ActionMenu } from "@react-spectrum/s2";

<div>
<ActionMenu shouldCloseOnSelect>
<MenuItem>Cut</MenuItem>
<MenuItem>Copy</MenuItem>
<MenuItem>Paste</MenuItem>
</ActionMenu>
</div>"
`;

exports[`Static - no change 1`] = `
"import { MenuItem, ActionMenu } from "@react-spectrum/s2";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,19 @@ exports[`Renames ContextualHelpTrigger to UnavailableMenuItemTrigger and Dialog
</MenuTrigger>"
`;

exports[`Renames closeOnSelect to shouldCloseOnSelect and moves to Menu 1`] = `
"import { MenuItem, Menu, MenuTrigger, Button } from "@react-spectrum/s2";

<MenuTrigger>
<Button>Edit</Button>
<Menu shouldCloseOnSelect>
<MenuItem>Cut</MenuItem>
<MenuItem>Copy</MenuItem>
<MenuItem>Paste</MenuItem>
</Menu>
</MenuTrigger>"
`;

exports[`Static - Renames Item to MenuItem, Section to MenuSection 1`] = `
"import {
MenuItem,
Expand Down
12 changes: 12 additions & 0 deletions packages/dev/codemods/src/s1-to-s2/__tests__/actionmenu.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,15 @@ let actionMenuItems = [
</ActionMenu>
</div>
`);

test('Renames closeOnSelect to shouldCloseOnSelect', `
import {ActionMenu, Item} from '@adobe/react-spectrum';

<div>
<ActionMenu closeOnSelect>
<Item>Cut</Item>
<Item>Copy</Item>
<Item>Paste</Item>
</ActionMenu>
</div>
`);
13 changes: 13 additions & 0 deletions packages/dev/codemods/src/s1-to-s2/__tests__/menu.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,16 @@ import {Menu, MenuTrigger, ContextualHelpTrigger, Item, Button, Dialog, Heading,
</Menu>
</MenuTrigger>
`);

test('Renames closeOnSelect to shouldCloseOnSelect and moves to Menu', `
import {Menu, MenuTrigger, Item, Button} from '@adobe/react-spectrum';

<MenuTrigger closeOnSelect>
<Button>Edit</Button>
<Menu>
<Item>Cut</Item>
<Item>Copy</Item>
<Item>Paste</Item>
</Menu>
</MenuTrigger>
`);
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import {commentOutProp} from '../../shared/transforms';
import {commentOutProp, updatePropName} from '../../shared/transforms';
import {NodePath} from '@babel/traverse';
import * as t from '@babel/types';

/**
* Transforms ActionMenu:
* - Comment out closeOnSelect (it has not been implemented yet).
* - Rename `closeOnSelect` to `shouldCloseOnSelect`.
* - Comment out trigger (it has not been implemented yet).
*/
export default function transformActionMenu(path: NodePath<t.JSXElement>): void {
// Comment out closeOnSelect
commentOutProp(path, {propName: 'closeOnSelect'});
// Rename `closeOnSelect` to `shouldCloseOnSelect`
updatePropName(path, {oldPropName: 'closeOnSelect', newPropName: 'shouldCloseOnSelect'});

// Comment out trigger
commentOutProp(path, {propName: 'trigger'});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import {movePropToChildComponent, updatePropName} from '../../shared/transforms';
import {NodePath} from '@babel/traverse';
import * as t from '@babel/types';

/**
* Transforms MenuTrigger:
* - Rename `closeOnSelect` to `shouldCloseOnSelect` and move it to the child `Menu`.
*/
export default function transformMenuTrigger(path: NodePath<t.JSXElement>): void {
updatePropName(path, {oldPropName: 'closeOnSelect', newPropName: 'shouldCloseOnSelect'});

movePropToChildComponent(path, {
parentComponentName: 'MenuTrigger',
childComponentName: 'Menu',
propName: 'shouldCloseOnSelect'
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,57 @@ export function movePropToParentComponent(
});
}

/**
* Moves a prop from the parent component onto a direct child JSX element.
*
* Example:
* - MenuTrigger: Remove `shouldCloseOnSelect` and add it to the child `Menu` instead.
*/
export function movePropToChildComponent(
path: NodePath<t.JSXElement>,
options: {
parentComponentName: string,
childComponentName: string,
propName: string
}
): void {
const {parentComponentName, childComponentName, propName} = options;

if (
!t.isJSXIdentifier(path.node.openingElement.name) ||
getName(path, path.node.openingElement.name) !== parentComponentName
) {
return;
}

let attrs = path.node.openingElement.attributes;
let propAttr = attrs.find(
(attr): attr is t.JSXAttribute => t.isJSXAttribute(attr) && attr.name.name === propName
);
if (!propAttr) {
return;
}

let childPath = path.get('children').find(
(child) =>
child.isJSXElement() &&
t.isJSXIdentifier(child.node.openingElement.name) &&
getName(path, child.node.openingElement.name) === childComponentName
);

if (!childPath?.isJSXElement()) {
return;
}

childPath.node.openingElement.attributes.push(
t.jsxAttribute(t.jsxIdentifier(propName), propAttr.value)
);
let index = attrs.indexOf(propAttr);
if (index !== -1) {
attrs.splice(index, 1);
}
}

/**
* Update to use a new component.
*
Expand Down
2 changes: 1 addition & 1 deletion packages/dev/s2-docs/pages/s2/migrating.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ No updates needed.

### MenuTrigger

- <PendingBadge /> Comment out `closeOnSelect` (it has not been implemented yet)
- Rename `closeOnSelect` to `shouldCloseOnSelect` and move to `Menu`

### NumberField

Expand Down
32 changes: 17 additions & 15 deletions packages/react-aria/src/color/useColorWheel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,21 +237,23 @@ export function useColorWheel(props: AriaColorWheelOptions, state: ColorWheelSta
}, movePropsContainer);

let thumbInteractions = isDisabled ? {} : mergeProps({
onMouseDown: (e: React.MouseEvent) => {
if (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey) {
return;
}
onThumbDown(undefined);
},
onPointerDown: (e: React.PointerEvent) => {
if (e.pointerType === 'mouse' && (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey)) {
return;
}
onThumbDown(e.pointerId);
},
onTouchStart: (e: React.TouchEvent) => {
onThumbDown(e.changedTouches[0].identifier);
}
...(typeof PointerEvent !== 'undefined' ? {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

weird that always providing each of those handlers caused an issue in iOS, but I guess this follows how we did this for track interactions. What was the exact sequence of event that was clashing here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not sure, i just matched colorArea
these are compat events anyways, so i'm guessing we just aren't canceling those

onPointerDown: (e: React.PointerEvent) => {
if (e.pointerType === 'mouse' && (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey)) {
return;
}
onThumbDown(e.pointerId);
}} : {
onMouseDown: (e: React.MouseEvent) => {
if (e.button !== 0 || e.altKey || e.ctrlKey || e.metaKey) {
return;
}
onThumbDown(undefined);
},
onTouchStart: (e: React.TouchEvent) => {
onThumbDown(e.changedTouches[0].identifier);
}
})
}, keyboardProps, movePropsThumb);
let {x, y} = state.getThumbPosition(thumbRadius);

Expand Down
Loading