Add isRootDropTarget parameter to renderEmptyState in GridList component #5211

gennadiipel · 2023-10-06T13:27:34Z

Add isRootDropTarget parameter to renderEmptyState in GridList component

Closes #5210

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

reidbarber

I like this idea a lot!

I say we go ahead and pass all the render props in (except for isEmpty since it is implied). And we can use one argument with everything.

reidbarber · 2023-10-06T14:58:42Z

packages/react-aria-components/src/GridList.tsx

@@ -176,7 +176,7 @@ function GridListInner<T extends object>({props, collection, gridListRef: ref}:
    // they don't affect the layout of the children. However, WebKit currently has
    // a bug that makes grid elements with display: contents hidden to screen readers.
    // https://bugs.webkit.org/show_bug.cgi?id=239479
-    let content = props.renderEmptyState();
+    let content = props.renderEmptyState(isRootDropTarget);


Suggested change

let content = props.renderEmptyState(isRootDropTarget);

let content = props.renderEmptyState({

isDropTarget: isRootDropTarget,

isFocused,

isFocusVisible,

state

});

reidbarber · 2023-10-06T15:03:20Z

packages/react-aria-components/src/GridList.tsx

@@ -54,7 +54,7 @@ export interface GridListProps<T> extends Omit<AriaGridListProps<T>, 'children'>
  /** The drag and drop hooks returned by `useDragAndDrop` used to enable drag and drop behavior for the GridList. */
  dragAndDropHooks?: DragAndDropHooks,
  /** Provides content to display when there are no items in the list. */
-  renderEmptyState?: () => ReactNode
+  renderEmptyState?: (isRootDropTarget: boolean) => ReactNode


Suggested change

renderEmptyState?: (isRootDropTarget: boolean) => ReactNode

renderEmptyState?: (props: {isDropTarget: boolean, isFocused: boolean, isFocusVisible: boolean, state: ListState<unknown>}) => ReactNode

reidbarber · 2023-10-06T16:09:48Z

GET_BUILD

rspbot · 2023-10-06T16:24:43Z

Build successful! 🎉

snowystinger · 2023-10-08T21:58:24Z

packages/react-aria-components/src/GridList.tsx

@@ -176,7 +176,12 @@ function GridListInner<T extends object>({props, collection, gridListRef: ref}:
    // they don't affect the layout of the children. However, WebKit currently has
    // a bug that makes grid elements with display: contents hidden to screen readers.
    // https://bugs.webkit.org/show_bug.cgi?id=239479
-    let content = props.renderEmptyState();
+    let content = props.renderEmptyState({


seems like a fine approach to me
we will need this in ListBox, Table, and TagGroup as well in order to be consistent
also, some form of a test/storybook example would be appreciated if you do not mind https://react-spectrum.adobe.com/contribute.html#pull-requests

gennadiipel · 2023-10-09T21:45:50Z

@snowystinger @reidbarber Hello, I updated Table, ListBox and TagGroup. Also updated docs a little and added example of reorderable tables including PR's changes to storybook. Please take a look.

snowystinger · 2023-10-09T22:28:28Z

GET_BUILD

rspbot · 2023-10-09T22:42:34Z

Build successful! 🎉

snowystinger · 2023-10-10T03:14:59Z

packages/react-aria-components/docs/ListBox.mdx

@@ -850,7 +850,7 @@ Use the `renderEmptyState` prop to customize what the `ListBox` will display if
 <ListBox
  aria-label="Search results"
  /*- begin highlight -*/
-  renderEmptyState={() => 'No results found.'}
+  renderEmptyState={({isDropTarget, state, isFocused, isFocusVisible}) => <span style={{color: isDropTarget ? 'red' : 'black'}}>No results found.</span>}


these are great, though in dark mode, it's a little hard to see

Instead, we should use one of the variables available to us in our docs, --spectrum-alias-text-color
There is another issue, though: I cannot drop anything on this example, so it never goes to red.

As a result, I think it should be in the example a little further down https://github.com/adobe/react-spectrum/blob/main/packages/react-aria-components/docs/ListBox.mdx#L1101
And instead of changing colors (esp to red which is negative), let's swap the string. Something like

isDropTarget ? 'This will not be uploaded' : 'Drop items here'

Yep, updated docs like that in all of the cases and also added for GridList (forgot this one)

reidbarber · 2023-10-10T14:11:12Z

GET_BUILD

rspbot · 2023-10-10T14:24:47Z

Build successful! 🎉

rspbot · 2023-10-10T14:24:50Z

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

reidbarber

LGTM, thanks!!

Add isRootDropTarget to GridList

1d5ad68

gennadiipel mentioned this pull request Oct 6, 2023

Add isRootDropTarget parameter to renderEmptyState in GridList component #5210

Closed

reidbarber requested changes Oct 6, 2023

View reviewed changes

gennadiipel requested a review from reidbarber October 6, 2023 15:49

Add more properties to renderEmptyState

0c48238

gennadiipel force-pushed the add-is-root-drop-target-to-gridlist branch from 2231b33 to 0c48238 Compare October 6, 2023 16:00

reidbarber previously approved these changes Oct 6, 2023

View reviewed changes

snowystinger reviewed Oct 8, 2023

View reviewed changes

gennadiipel dismissed reidbarber’s stale review via cbd6aaf October 9, 2023 20:02

gennadiipel force-pushed the add-is-root-drop-target-to-gridlist branch from cbd6aaf to 0c48238 Compare October 9, 2023 20:14

gennadiipel and others added 5 commits October 9, 2023 23:15

Add props to ListBox, Table and TagGroup

8eab93e

Add ReordableTableExample to storybook

917d3b9

WIP add fixes to types

8832a24

Merge branch 'main' into add-is-root-drop-target-to-gridlist

5726f24

Update docs

62bd780

gennadiipel force-pushed the add-is-root-drop-target-to-gridlist branch from 5982fb7 to 62bd780 Compare October 9, 2023 21:34

gennadiipel requested review from snowystinger and reidbarber October 9, 2023 21:42

Fix typo in Reorderable tables

7305a24

snowystinger reviewed Oct 10, 2023

View reviewed changes

gennadiipel requested a review from snowystinger October 10, 2023 08:17

gennadiipel force-pushed the add-is-root-drop-target-to-gridlist branch from caa1271 to ccd1870 Compare October 10, 2023 08:31

update docs

1f1576c

gennadiipel force-pushed the add-is-root-drop-target-to-gridlist branch from ccd1870 to 1f1576c Compare October 10, 2023 08:32

Merge branch 'main' into add-is-root-drop-target-to-gridlist

c70605b

reidbarber previously approved these changes Oct 10, 2023

View reviewed changes

snowystinger previously approved these changes Oct 10, 2023

View reviewed changes

reuse render props

51c0be2

snowystinger dismissed stale reviews from reidbarber and themself via 51c0be2 October 10, 2023 21:51

snowystinger added 3 commits October 11, 2023 09:27

changes from internal discussion

eae1036

fix lint

c9a1ae9

Merge branch 'main' into add-is-root-drop-target-to-gridlist

e9b4a8a

devongovett approved these changes Oct 11, 2023

View reviewed changes

Merge branch 'main' into add-is-root-drop-target-to-gridlist

9f75815

snowystinger approved these changes Oct 11, 2023

View reviewed changes

snowystinger merged commit 778eaa4 into adobe:main Oct 11, 2023
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isRootDropTarget parameter to renderEmptyState in GridList component #5211

Add isRootDropTarget parameter to renderEmptyState in GridList component #5211

gennadiipel commented Oct 6, 2023 •

edited

Loading

reidbarber left a comment

reidbarber Oct 6, 2023

reidbarber Oct 6, 2023

reidbarber commented Oct 6, 2023

rspbot commented Oct 6, 2023

snowystinger Oct 8, 2023

gennadiipel commented Oct 9, 2023

snowystinger commented Oct 9, 2023

rspbot commented Oct 9, 2023

snowystinger Oct 10, 2023

gennadiipel Oct 10, 2023

reidbarber commented Oct 10, 2023

rspbot commented Oct 10, 2023

rspbot commented Oct 10, 2023

reidbarber left a comment

	renderEmptyState?: (isRootDropTarget: boolean) => ReactNode
	renderEmptyState?: (props: {isDropTarget: boolean, isFocused: boolean, isFocusVisible: boolean, state: ListState<unknown>}) => ReactNode

Add isRootDropTarget parameter to renderEmptyState in GridList component #5211

Add isRootDropTarget parameter to renderEmptyState in GridList component #5211

Conversation

gennadiipel commented Oct 6, 2023 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

reidbarber left a comment

Choose a reason for hiding this comment

reidbarber Oct 6, 2023

Choose a reason for hiding this comment

reidbarber Oct 6, 2023

Choose a reason for hiding this comment

reidbarber commented Oct 6, 2023

rspbot commented Oct 6, 2023

snowystinger Oct 8, 2023

Choose a reason for hiding this comment

gennadiipel commented Oct 9, 2023

snowystinger commented Oct 9, 2023

rspbot commented Oct 9, 2023

snowystinger Oct 10, 2023

Choose a reason for hiding this comment

gennadiipel Oct 10, 2023

Choose a reason for hiding this comment

reidbarber commented Oct 10, 2023

rspbot commented Oct 10, 2023

rspbot commented Oct 10, 2023

reidbarber left a comment

Choose a reason for hiding this comment

gennadiipel commented Oct 6, 2023 •

edited

Loading