From 60c167b44c062ba175e46901c5e0578ed03ac635 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Mon, 2 Mar 2020 13:21:24 -0800 Subject: [PATCH 1/4] Support variable heights and self sizing collection views --- .../components/menu/index.css | 11 +--- .../collections/src/CollectionView.tsx | 28 +++++++-- .../collections/src/ScrollView.tsx | 25 +++++++- packages/@react-aria/collections/src/index.ts | 1 + .../collections/src/useCollectionItem.ts | 39 ++++++++++++ packages/@react-spectrum/menu/src/Menu.tsx | 19 +++--- packages/@react-spectrum/menu/src/index.ts | 3 - .../@react-spectrum/typography/src/Text.tsx | 4 +- .../collections/src/CollectionManager.ts | 34 +++-------- .../collections/src/ListLayout.ts | 61 ++++++++++++++++--- .../collections/src/ReusableView.ts | 9 ++- .../collections/src/Transaction.ts | 2 +- .../@react-stately/collections/src/types.ts | 4 +- 13 files changed, 175 insertions(+), 65 deletions(-) create mode 100644 packages/@react-aria/collections/src/useCollectionItem.ts diff --git a/packages/@adobe/spectrum-css-temp/components/menu/index.css b/packages/@adobe/spectrum-css-temp/components/menu/index.css index 79945558358..08baef85fd7 100644 --- a/packages/@adobe/spectrum-css-temp/components/menu/index.css +++ b/packages/@adobe/spectrum-css-temp/components/menu/index.css @@ -46,8 +46,8 @@ governing permissions and limitations under the License. overflow: auto; - height: 255px; width: 200px; + max-height: inherit; /* inherit from parent popover */ & .spectrum-Menu-sectionHeading { /* Support headings as LI */ @@ -111,13 +111,6 @@ governing permissions and limitations under the License. .spectrum-Menu-itemLabel { grid-area: text; align-self: center; - line-height: var(--spectrum-selectlist-option-label-line-height); - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - /* Override user agentstyle sheet for

*/ - margin-block-start: 0px; - margin-block-end: 0px; } .spectrum-Menu-itemLabel--wrapping { @@ -165,8 +158,6 @@ governing permissions and limitations under the License. } .spectrum-Menu-itemGrid { - /* hard coded height to match row height in Menu.tsx */ - height: 32px; display: grid; grid-template-columns: calc(var(--spectrum-selectlist-option-padding) - var(--spectrum-selectlist-border-size-key-focus)) auto 1fr auto auto var(--spectrum-selectlist-option-padding); grid-template-rows: var(--spectrum-selectlist-option-padding-y) 1fr auto var(--spectrum-selectlist-option-padding-y); diff --git a/packages/@react-aria/collections/src/CollectionView.tsx b/packages/@react-aria/collections/src/CollectionView.tsx index cc3334e9de7..ec721a90f36 100644 --- a/packages/@react-aria/collections/src/CollectionView.tsx +++ b/packages/@react-aria/collections/src/CollectionView.tsx @@ -14,17 +14,19 @@ import {chain} from '@react-aria/utils'; import {Collection, Layout, LayoutInfo} from '@react-stately/collections'; import React, {CSSProperties, FocusEvent, HTMLAttributes, Key, useCallback, useEffect, useRef} from 'react'; import {ScrollView} from './ScrollView'; +import {useCollectionItem} from './useCollectionItem'; import {useCollectionState} from '@react-stately/collections'; interface CollectionViewProps extends HTMLAttributes { children: (type: string, content: T) => V, layout: Layout, collection: Collection, - focusedKey?: Key + focusedKey?: Key, + sizeToFit?: 'width' | 'height' } export function CollectionView(props: CollectionViewProps) { - let {children: renderView, layout, collection, focusedKey, ...otherProps} = props; + let {children: renderView, layout, collection, focusedKey, sizeToFit, ...otherProps} = props; let { visibleViews, visibleRect, @@ -37,9 +39,9 @@ export function CollectionView(props: CollectionViewProps ( -

+ {reusableView.rendered} -
+ ) }); @@ -90,12 +92,28 @@ export function CollectionView(props: CollectionViewProps + onVisibleRectChange={setVisibleRect} + sizeToFit={sizeToFit}> {visibleViews} ); } +function CollectionItem({layoutInfo, collectionManager, children}) { + let ref = useRef(); + useCollectionItem({ + layoutInfo, + collectionManager, + ref + }); + + return ( +
+ {children} +
+ ); +} + function layoutInfoToStyle(layoutInfo: LayoutInfo): CSSProperties { return { position: 'absolute', diff --git a/packages/@react-aria/collections/src/ScrollView.tsx b/packages/@react-aria/collections/src/ScrollView.tsx index 9e327d9ade8..2044f247ab0 100644 --- a/packages/@react-aria/collections/src/ScrollView.tsx +++ b/packages/@react-aria/collections/src/ScrollView.tsx @@ -19,7 +19,8 @@ interface ScrollViewProps extends HTMLAttributes { visibleRect: Rect, onVisibleRectChange: (rect: Rect) => void, children: ReactNode, - innerStyle: CSSProperties + innerStyle: CSSProperties, + sizeToFit: 'width' | 'height' } function ScrollView(props: ScrollViewProps, ref: RefObject) { @@ -29,6 +30,7 @@ function ScrollView(props: ScrollViewProps, ref: RefObject) { onVisibleRectChange, children, innerStyle, + sizeToFit, ...otherProps } = props; @@ -81,6 +83,25 @@ function ScrollView(props: ScrollViewProps, ref: RefObject) { let w = dom.offsetWidth; let h = dom.offsetHeight; + if (sizeToFit) { + let style = window.getComputedStyle(dom); + + if (sizeToFit === 'width') { + w = contentSize.width; + + let maxWidth = parseInt(style.maxWidth, 10); + if (!isNaN(maxWidth)) { + w = Math.min(maxWidth, w); + } + } else if (sizeToFit === 'height') { + h = contentSize.height; + + let maxHeight = parseInt(style.maxHeight, 10); + if (!isNaN(maxHeight)) { + h = Math.min(maxHeight, h); + } + } + } if (state.width !== w || state.height !== h) { state.width = w; @@ -94,7 +115,7 @@ function ScrollView(props: ScrollViewProps, ref: RefObject) { return () => { window.removeEventListener('resize', updateSize, false); }; - }, [onVisibleRectChange, ref, state.height, state.scrollLeft, state.scrollTop, state.width]); + }, [onVisibleRectChange, ref, state.height, state.scrollLeft, state.scrollTop, state.width, sizeToFit, contentSize.width, contentSize.height]); useLayoutEffect(() => { let dom = ref.current; diff --git a/packages/@react-aria/collections/src/index.ts b/packages/@react-aria/collections/src/index.ts index 42da807af32..8a44f8a269a 100644 --- a/packages/@react-aria/collections/src/index.ts +++ b/packages/@react-aria/collections/src/index.ts @@ -11,3 +11,4 @@ */ export * from './CollectionView'; +export * from './useCollectionItem'; diff --git a/packages/@react-aria/collections/src/useCollectionItem.ts b/packages/@react-aria/collections/src/useCollectionItem.ts new file mode 100644 index 00000000000..b6e18640583 --- /dev/null +++ b/packages/@react-aria/collections/src/useCollectionItem.ts @@ -0,0 +1,39 @@ +import {CollectionManager, LayoutInfo, Size} from '@react-stately/collections'; +import {RefObject, useCallback, useLayoutEffect} from 'react'; + +interface CollectionItemOptions { + layoutInfo: LayoutInfo, + collectionManager: CollectionManager, + ref: RefObject +} + +export function useCollectionItem(options: CollectionItemOptions) { + let {layoutInfo, collectionManager, ref} = options; + + let updateSize = useCallback(() => { + let size = getSize(ref.current); + collectionManager.updateItemSize(layoutInfo.key, size); + }, [collectionManager, layoutInfo.key, ref]); + + useLayoutEffect(() => { + if (layoutInfo.estimatedSize) { + updateSize(); + } + }); + + return {updateSize}; +} + +function getSize(node: HTMLElement) { + // Get bounding rect of all children + let top = Infinity, left = Infinity, bottom = 0, right = 0; + for (let child of Array.from(node.childNodes)) { + let rect = (child as HTMLElement).getBoundingClientRect(); + top = Math.min(top, rect.top); + left = Math.min(left, rect.left); + bottom = Math.max(bottom, rect.bottom); + right = Math.max(right, rect.right); + } + + return new Size(right - left, bottom - top); +} diff --git a/packages/@react-spectrum/menu/src/Menu.tsx b/packages/@react-spectrum/menu/src/Menu.tsx index d355773c952..6d5d1cc080a 100644 --- a/packages/@react-spectrum/menu/src/Menu.tsx +++ b/packages/@react-spectrum/menu/src/Menu.tsx @@ -12,30 +12,34 @@ import {classNames, filterDOMProps, useStyleProps} from '@react-spectrum/utils'; import {CollectionView} from '@react-aria/collections'; -import {Item, ListLayout, Node, Section} from '@react-stately/collections'; +import {Item, ListLayout, Section} from '@react-stately/collections'; import {MenuContext} from './context'; -import {MenuDivider, MenuHeading, MenuItem} from './'; +import {MenuDivider} from './MenuDivider'; +import {MenuHeading} from './MenuHeading'; +import {MenuItem} from './MenuItem'; import {mergeProps} from '@react-aria/utils'; import React, {Fragment, useContext, useMemo} from 'react'; import {SpectrumMenuProps} from '@react-types/menu'; import styles from '@adobe/spectrum-css-temp/components/menu/vars.css'; import {useMenu} from '@react-aria/menu'; +import {useProvider} from '@react-spectrum/provider'; import {useTreeState} from '@react-stately/tree'; export {Item, Section}; export function Menu(props: SpectrumMenuProps) { + let {scale} = useProvider(); let layout = useMemo(() => new ListLayout({ - rowHeight: 32, // Feel like we should eventually calculate this number (based on the css)? It should probably get a multiplier in order to gracefully handle scaling - headingHeight: 31 // Same as above + estimatedRowHeight: scale === 'large' ? 48 : 32, + estimatedHeadingHeight: scale === 'large' ? 31 : 25 }) - , []); + , [scale]); let contextProps = useContext(MenuContext); let completeProps = { ...mergeProps(contextProps, props), - selectionMode: props.selectionMode || 'single' + selectionMode: props.selectionMode || 'none' }; let state = useTreeState(completeProps); @@ -50,6 +54,7 @@ export function Menu(props: SpectrumMenuProps) { {...styleProps} {...menuProps} focusedKey={state.selectionManager.focusedKey} + sizeToFit="height" className={ classNames( styles, @@ -59,7 +64,7 @@ export function Menu(props: SpectrumMenuProps) { } layout={layout} collection={state.tree}> - {(type, item: Node) => { + {(type, item) => { if (type === 'section') { // Only render the Divider if it isn't the first Heading (extra equality check to guard against rerenders) if (item.key === state.tree.getKeys().next().value) { diff --git a/packages/@react-spectrum/menu/src/index.ts b/packages/@react-spectrum/menu/src/index.ts index fc2437a80d5..bcd60686725 100644 --- a/packages/@react-spectrum/menu/src/index.ts +++ b/packages/@react-spectrum/menu/src/index.ts @@ -10,8 +10,5 @@ * governing permissions and limitations under the License. */ -export * from './MenuDivider'; -export * from './MenuHeading'; -export * from './MenuItem'; export * from './MenuTrigger'; export * from './Menu'; diff --git a/packages/@react-spectrum/typography/src/Text.tsx b/packages/@react-spectrum/typography/src/Text.tsx index 2e0f05f57df..97966bf76a1 100644 --- a/packages/@react-spectrum/typography/src/Text.tsx +++ b/packages/@react-spectrum/typography/src/Text.tsx @@ -24,8 +24,8 @@ export const Text = React.forwardRef((props: TextProps, ref: RefObject + {children} -

+ ); }); diff --git a/packages/@react-stately/collections/src/CollectionManager.ts b/packages/@react-stately/collections/src/CollectionManager.ts index 8729c67ddf3..e7ae528cf5d 100644 --- a/packages/@react-stately/collections/src/CollectionManager.ts +++ b/packages/@react-stately/collections/src/CollectionManager.ts @@ -104,7 +104,7 @@ export class CollectionManager { private _overscanManager: OverscanManager; private _relayoutRaf: number | null; private _scrollAnimation: CancelablePromise | null; - private _sizeUpdateQueue: Set; + private _sizeUpdateQueue: Map; private _animatedContentOffset: Point; private _transaction: Transaction | null; private _nextTransaction: Transaction | null; @@ -123,7 +123,7 @@ export class CollectionManager { this._overscanManager = new OverscanManager(); this._scrollAnimation = null; - this._sizeUpdateQueue = new Set(); + this._sizeUpdateQueue = new Map(); this._animatedContentOffset = new Point(0, 0); this._transaction = null; @@ -313,7 +313,7 @@ export class CollectionManager { let reusable = this._reusableViews[reuseType]; let view = reusable.length > 0 ? reusable.pop() - : new ReusableView(); + : new ReusableView(this); view.viewType = reuseType; @@ -795,18 +795,10 @@ export class CollectionManager { } } - let needsSizeUpdate = false; - for (let key of toAdd.keys()) { let layoutInfo = visibleLayoutInfos.get(key); let view: ReusableView | void; - // We need to recompute view sizes if we only - // have an estimated size for this row. - if (layoutInfo.estimatedSize) { - needsSizeUpdate = true; - } - // If we're in a transaction, and a layout change happens // during the animations such that a view that was going // to be removed is now not, we don't create a new view @@ -850,19 +842,13 @@ export class CollectionManager { } this._flushVisibleViews(); - if (this._transaction || needsSizeUpdate) { + if (this._transaction) { requestAnimationFrame(() => { // If we're in a transaction, apply animations to visible views // and "to be removed" views, which animate off screen. if (this._transaction) { requestAnimationFrame(() => this._applyLayoutInfos()); } - - // If we need a height update, wait until the next frame - // so that the browser has time to do layout. - if (needsSizeUpdate) { - this.relayout(); - } }); } } @@ -947,7 +933,7 @@ export class CollectionManager { } } - updateItemSize(key: Key) { + updateItemSize(key: Key, size: Size) { // TODO: we should be able to invalidate a single index path // @ts-ignore if (!this.layout.updateItemSize) { @@ -957,14 +943,14 @@ export class CollectionManager { // If the scroll position is currently animating, add the update // to a queue to be processed after the animation is complete. if (this._scrollAnimation) { - this._sizeUpdateQueue.add(key); + this._sizeUpdateQueue.set(key, size); return; } // @ts-ignore - let changed = this.layout.updateItemSize(key); + let changed = this.layout.updateItemSize(key, size); if (changed) { - this.relayout(); + this.relayoutNow(); } } @@ -1042,8 +1028,8 @@ export class CollectionManager { // Process view size updates that occurred during the animation. // Only views that are still visible will be actually updated. - for (let key of this._sizeUpdateQueue) { - this.updateItemSize(key); + for (let [key, size] of this._sizeUpdateQueue) { + this.updateItemSize(key, size); } this._sizeUpdateQueue.clear(); diff --git a/packages/@react-stately/collections/src/ListLayout.ts b/packages/@react-stately/collections/src/ListLayout.ts index 1f67326c98d..1613420bc5d 100644 --- a/packages/@react-stately/collections/src/ListLayout.ts +++ b/packages/@react-stately/collections/src/ListLayout.ts @@ -10,7 +10,7 @@ * governing permissions and limitations under the License. */ -import {Collection, Node} from './types'; +import {Collection, InvalidationContext, Node} from './types'; import {Key} from 'react'; import {KeyboardDelegate} from '@react-types/shared'; import {Layout} from './Layout'; @@ -23,10 +23,14 @@ import {Size} from './Size'; type ListLayoutOptions = { /** the height of a row in px. */ rowHeight?: number, + estimatedRowHeight?: number, headingHeight?: number, + estimatedHeadingHeight?: number, indentationForItem?: (collection: Collection>, key: Key) => number }; +const DEFAULT_HEIGHT = 48; + /** * The ListLayout class is an implementation of a collection view {@link Layout} * it is used for creating lists and lists with indented sub-lists @@ -39,10 +43,13 @@ type ListLayoutOptions = { */ export class ListLayout extends Layout> implements KeyboardDelegate { private rowHeight: number; + private estimatedRowHeight: number; private headingHeight: number; + private estimatedHeadingHeight: number; private indentationForItem?: (collection: Collection>, key: Key) => number; private layoutInfos: {[key: string]: LayoutInfo}; private contentHeight: number; + private lastCollection: Collection>; /** * Creates a new ListLayout with options. See the list of properties below for a description @@ -50,10 +57,13 @@ export class ListLayout extends Layout> implements KeyboardDelegate { */ constructor(options: ListLayoutOptions = {}) { super(); - this.rowHeight = options.rowHeight || 48; - this.headingHeight = options.headingHeight || 48; + this.rowHeight = options.rowHeight; + this.estimatedRowHeight = options.estimatedRowHeight; + this.headingHeight = options.headingHeight; + this.estimatedHeadingHeight = options.estimatedHeadingHeight; this.indentationForItem = options.indentationForItem; this.layoutInfos = {}; + this.lastCollection = null; this.contentHeight = 0; } @@ -73,29 +83,66 @@ export class ListLayout extends Layout> implements KeyboardDelegate { return res; } - validate() { + validate(invalidationContext: InvalidationContext) { + let previousLayoutInfos = this.layoutInfos; this.layoutInfos = {}; let y = 0; let keys = this.collectionManager.collection.getKeys(); for (let key of keys) { - let type = this.collectionManager.collection.getItem(key).type; - let rectHeight = type === 'item' ? this.rowHeight : this.headingHeight; + let node = this.collectionManager.collection.getItem(key); + let rectHeight = node.type === 'item' ? this.rowHeight : this.headingHeight; + let isEstimated = false; + + // If no explicit height is available, use an estimated height. + if (rectHeight == null) { + // If a previous version of this layout info exists, reuse its height. + // Mark as estimated if the size of the overall collection view changed, + // or the content of the item changed. + let previousLayoutInfo = previousLayoutInfos[key]; + if (previousLayoutInfo) { + let lastNode = this.lastCollection ? this.lastCollection.getItem(key) : null; + rectHeight = previousLayoutInfo.rect.height; + isEstimated = invalidationContext.sizeChanged || node !== lastNode || previousLayoutInfo.estimatedSize; + } else { + rectHeight = node.type === 'item' ? this.estimatedRowHeight : this.estimatedHeadingHeight; + isEstimated = true; + } + } + + if (rectHeight == null) { + rectHeight = DEFAULT_HEIGHT; + } + let x = 0; if (typeof this.indentationForItem === 'function') { x = this.indentationForItem(this.collectionManager.collection, key) || 0; } let rect = new Rect(x, y, this.collectionManager.visibleRect.width - x, rectHeight); - this.layoutInfos[key] = new LayoutInfo(type, key, rect); + let layoutInfo = new LayoutInfo(node.type, key, rect); + layoutInfo.estimatedSize = isEstimated; + this.layoutInfos[key] = layoutInfo; y += rectHeight; } + this.lastCollection = this.collectionManager.collection; this.contentHeight = y; } + updateItemSize(key: Key, size: Size) { + let layoutInfo = this.layoutInfos[key]; + layoutInfo.estimatedSize = false; + if (layoutInfo.rect.height !== size.height) { + layoutInfo.rect.height = size.height; + return true; + } + + return false; + } + getContentSize() { return new Size(this.collectionManager.visibleRect.width, this.contentHeight); } diff --git a/packages/@react-stately/collections/src/ReusableView.ts b/packages/@react-stately/collections/src/ReusableView.ts index d3a32d46480..d0ae3da3e78 100644 --- a/packages/@react-stately/collections/src/ReusableView.ts +++ b/packages/@react-stately/collections/src/ReusableView.ts @@ -10,6 +10,7 @@ * governing permissions and limitations under the License. */ +import {CollectionManager} from './CollectionManager'; import {Key} from 'react'; import {LayoutInfo} from './LayoutInfo'; @@ -22,7 +23,10 @@ let KEY = 0; * as needed. Subclasses must implement the {@link render} method at a * minimum. Other methods can be overrided to customize behavior. */ -export class ReusableView { +export class ReusableView { + /** The CollectionManager this view is a part of */ + collectionManager: CollectionManager; + /** The LayoutInfo this view is currently representing. */ layoutInfo: LayoutInfo | null; @@ -34,7 +38,8 @@ export class ReusableView { viewType: string; key: Key; - constructor() { + constructor(collectionManager: CollectionManager) { + this.collectionManager = collectionManager; this.key = ++KEY; } diff --git a/packages/@react-stately/collections/src/Transaction.ts b/packages/@react-stately/collections/src/Transaction.ts index 8b560f03633..e2994e18b68 100644 --- a/packages/@react-stately/collections/src/Transaction.ts +++ b/packages/@react-stately/collections/src/Transaction.ts @@ -15,7 +15,7 @@ import {LayoutInfo} from './LayoutInfo'; import {ReusableView} from './ReusableView'; type LayoutInfoMap = Map; -export class Transaction { +export class Transaction { level = 0; actions: (() => void)[] = []; animated = true; diff --git a/packages/@react-stately/collections/src/types.ts b/packages/@react-stately/collections/src/types.ts index d5b6b0b4dd4..5f88a455819 100644 --- a/packages/@react-stately/collections/src/types.ts +++ b/packages/@react-stately/collections/src/types.ts @@ -44,7 +44,7 @@ export interface Collection { getLastKey(): Key | null } -export interface InvalidationContext { +export interface InvalidationContext { contentChanged?: boolean, offsetChanged?: boolean, sizeChanged?: boolean, @@ -55,7 +55,7 @@ export interface InvalidationContext { transaction?: Transaction } -export interface CollectionManagerDelegate { +export interface CollectionManagerDelegate { setVisibleViews(views: Set): void, setContentSize(size: Size): void, setVisibleRect(rect: Rect): void, From 09a62ab2b71dd76840b7faf51006e618b3807a81 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Mon, 2 Mar 2020 13:45:09 -0800 Subject: [PATCH 2/4] Fix tests --- packages/@react-aria/collections/src/ScrollView.tsx | 2 +- packages/@react-spectrum/menu/src/Menu.tsx | 2 +- packages/@react-spectrum/menu/test/Menu.test.js | 4 ---- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/@react-aria/collections/src/ScrollView.tsx b/packages/@react-aria/collections/src/ScrollView.tsx index 2044f247ab0..a6026ba9af8 100644 --- a/packages/@react-aria/collections/src/ScrollView.tsx +++ b/packages/@react-aria/collections/src/ScrollView.tsx @@ -83,7 +83,7 @@ function ScrollView(props: ScrollViewProps, ref: RefObject) { let w = dom.offsetWidth; let h = dom.offsetHeight; - if (sizeToFit) { + if (sizeToFit && contentSize.width > 0 && contentSize.height > 0) { let style = window.getComputedStyle(dom); if (sizeToFit === 'width') { diff --git a/packages/@react-spectrum/menu/src/Menu.tsx b/packages/@react-spectrum/menu/src/Menu.tsx index 6d5d1cc080a..934c012607b 100644 --- a/packages/@react-spectrum/menu/src/Menu.tsx +++ b/packages/@react-spectrum/menu/src/Menu.tsx @@ -39,7 +39,7 @@ export function Menu(props: SpectrumMenuProps) { let contextProps = useContext(MenuContext); let completeProps = { ...mergeProps(contextProps, props), - selectionMode: props.selectionMode || 'none' + selectionMode: props.selectionMode || 'single' }; let state = useTreeState(completeProps); diff --git a/packages/@react-spectrum/menu/test/Menu.test.js b/packages/@react-spectrum/menu/test/Menu.test.js index 96eeef2d0a0..1906dda495a 100644 --- a/packages/@react-spectrum/menu/test/Menu.test.js +++ b/packages/@react-spectrum/menu/test/Menu.test.js @@ -160,7 +160,6 @@ describe('Menu', function () { ${'V2Menu'} | ${V2Menu} | ${{}} `('$Name allows user to change menu item focus via up/down arrow keys', async function ({Component, props}) { let tree = renderComponent(Component, {}, props); - await waitForDomChange(); let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[0]; @@ -178,7 +177,6 @@ describe('Menu', function () { ${'Menu'} | ${Menu} | ${{autoFocus: true, wrapAround: true}} `('$Name wraps focus from first to last/last to first item if up/down arrow is pressed if wrapAround is true', async function ({Component, props}) { let tree = renderComponent(Component, {}, props); - await waitForDomChange(); let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let firstItem = menuItems[0]; @@ -214,7 +212,6 @@ describe('Menu', function () { `('$Name supports defaultSelectedKeys (uncontrolled)', async function ({Component, props}) { // Check that correct menu item is selected by default let tree = renderComponent(Component, {}, props); - await waitForDomChange(); let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[3]; @@ -249,7 +246,6 @@ describe('Menu', function () { `('$Name supports selectedKeys (controlled)', async function ({Component, props}) { // Check that correct menu item is selected by default let tree = renderComponent(Component, {}, props); - await waitForDomChange(); let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[3]; From d610e11577e3ab1ff85504927fcb8f4fc6a6c14a Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Mon, 2 Mar 2020 13:53:43 -0800 Subject: [PATCH 3/4] awaitDomChange in v2 only --- packages/@react-spectrum/menu/test/Menu.test.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/packages/@react-spectrum/menu/test/Menu.test.js b/packages/@react-spectrum/menu/test/Menu.test.js index 1906dda495a..b37c4428543 100644 --- a/packages/@react-spectrum/menu/test/Menu.test.js +++ b/packages/@react-spectrum/menu/test/Menu.test.js @@ -160,6 +160,9 @@ describe('Menu', function () { ${'V2Menu'} | ${V2Menu} | ${{}} `('$Name allows user to change menu item focus via up/down arrow keys', async function ({Component, props}) { let tree = renderComponent(Component, {}, props); + if (Component === V2Menu) { + await waitForDomChange(); + } let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[0]; @@ -177,6 +180,9 @@ describe('Menu', function () { ${'Menu'} | ${Menu} | ${{autoFocus: true, wrapAround: true}} `('$Name wraps focus from first to last/last to first item if up/down arrow is pressed if wrapAround is true', async function ({Component, props}) { let tree = renderComponent(Component, {}, props); + if (Component === V2Menu) { + await waitForDomChange(); + } let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let firstItem = menuItems[0]; @@ -212,6 +218,9 @@ describe('Menu', function () { `('$Name supports defaultSelectedKeys (uncontrolled)', async function ({Component, props}) { // Check that correct menu item is selected by default let tree = renderComponent(Component, {}, props); + if (Component === V2Menu) { + await waitForDomChange(); + } let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[3]; @@ -246,6 +255,9 @@ describe('Menu', function () { `('$Name supports selectedKeys (controlled)', async function ({Component, props}) { // Check that correct menu item is selected by default let tree = renderComponent(Component, {}, props); + if (Component === V2Menu) { + await waitForDomChange(); + } let menu = tree.getByRole('menu'); let menuItems = within(menu).getAllByRole('menuitemradio'); let selectedItem = menuItems[3]; From 5d45c2bbb6d75b5fa56aa84f4a03b32d7517b621 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Mon, 2 Mar 2020 14:18:57 -0800 Subject: [PATCH 4/4] Make description wrap --- packages/@adobe/spectrum-css-temp/components/menu/index.css | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/@adobe/spectrum-css-temp/components/menu/index.css b/packages/@adobe/spectrum-css-temp/components/menu/index.css index 08baef85fd7..0acded71b46 100644 --- a/packages/@adobe/spectrum-css-temp/components/menu/index.css +++ b/packages/@adobe/spectrum-css-temp/components/menu/index.css @@ -186,12 +186,6 @@ governing permissions and limitations under the License. align-self: center; line-height: 1.3; font-size: var(--spectrum-global-dimension-size-150); - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - /* Override user agentstyle sheet for

*/ - margin-block-start: 0px; - margin-block-end: 0px; } .spectrum-Menu-keyboard { grid-area: keyboard;