Round ListView item borders in quiet #3105

snowystinger · 2022-05-05T17:12:09Z

Closes

✅ 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:

snowystinger · 2022-05-05T18:17:46Z

I've only implemented this for single selection mode so far. It should look like this:

Multiple selection mode is a little odd, off the bat it looks like this:

So the question is, do we want to do square corners for ranges but round the first and last for multiple? like this:

I've committed this so you can see it live as well

adobe-bot · 2022-05-05T18:25:14Z

Build successful! 🎉

adobe-bot · 2022-05-05T18:26:45Z

Build successful! 🎉

majornista · 2022-05-05T19:35:09Z

+1 for rounded corners on first and last of a group of selected items. Looks sophisticated.

LFDanLu

I definitely like the square corners for quiet + multiple selection when consecutive rows are selected. Should the hover style and keyboard focused but not selected style have rounded corners as well?
Hover style:

keyboard focus but not selected:

devongovett · 2022-05-07T00:00:41Z

packages/@react-spectrum/list/src/styles.css

  background-color: var(--spectrum-alias-background-color-transparent);
  border-width: 0;
  border-radius: 0;
+  &:not(.react-spectrum-ListView--multipleSelection) {


This is... quite the CSS. Not sure I follow completely, but there's no way this can be simplified?

I see a place i can simplify a little 👍🏻

devongovett · 2022-05-07T00:01:19Z

A weird case

# Conflicts: # packages/@react-spectrum/list/src/ListView.tsx # packages/@react-spectrum/list/src/styles.css

adobe-bot · 2022-05-07T04:57:33Z

Build successful! 🎉

adobe-bot · 2022-05-07T06:45:03Z

Build successful! 🎉

devongovett · 2022-05-09T15:59:50Z

packages/@react-spectrum/list/src/ListView.tsx


  let hasAnyChildren = useMemo(() => [...collection].some(item => item.hasChildNodes), [collection]);
+  let [hoveredKey, setHoveredKey] = useState<Key>();
+  let hoverState = {hoveredKey, setHoveredKey};


lol might be overkill but let's see what people think. IMO it's a little strange that the corners change when hovering over an adjacent item 😂

tried out in f39193e
which has this storybook URL https://reactspectrum.blob.core.windows.net/reactspectrum/f39193ea5b900737675e16dc5fcb061d05b9af2c/storybook/index.html

devongovett · 2022-05-09T16:01:55Z

packages/@react-spectrum/list/src/ListViewItem.tsx

 import type {DraggableItemResult} from '@react-aria/dnd';
 import {FocusRing, useFocusRing} from '@react-aria/focus';
 import {Grid} from '@react-spectrum/layout';
+import {isFocusVisible as isFocusVisibleFn, useHover, usePress} from '@react-aria/interactions';


isGlobalFocusVisible?

assuming this is just a naming thing, then I've renamed it

devongovett · 2022-05-09T16:03:17Z

packages/@react-spectrum/list/src/ListViewItem.tsx

-            'focus-ring': isFocusVisible
+            'focus-ring': isFocusVisible,
+            'round-tops': !state.selectionManager.isSelected(item.prevKey) && (state.selectionManager.focusedKey !== item.prevKey || !isFocusVisibleFn()) && hoverState.hoveredKey !== item.prevKey,
+            'round-bottoms': !state.selectionManager.isSelected(item.nextKey) && (state.selectionManager.focusedKey !== item.nextKey || !isFocusVisibleFn()) && hoverState.hoveredKey !== item.nextKey


I think the focus visible behavior isn't quite right. It shouldn't be if anything on the page has keyboard modality, it should only be if focus is somewhere inside the listview. Right now if you select something and then tab away from the listview the corners aren't rounded anymore.

fixed in 540adf1
good catch

adobe-bot · 2022-05-09T16:52:49Z

Build successful! 🎉

adobe-bot · 2022-05-09T22:15:07Z

Build successful! 🎉

adobe-bot · 2022-05-09T22:21:45Z

Build successful! 🎉

adobe-bot · 2022-05-09T22:49:05Z

Build successful! 🎉

adobe-bot · 2022-05-09T22:52:23Z

Build successful! 🎉

devongovett · 2022-05-09T23:43:38Z

packages/@react-spectrum/list/src/ListView.tsx


  return (
-    <ListViewContext.Provider value={{state, dragState, isListDraggable, layout}}>
+    <ListViewContext.Provider value={{state, dragState, isListDraggable, layout, isFocusWithin}}>


I think selectionManager.isFocused should already have this info?

odd, i tried that, but i must have had a bad combo with something else, as it appears to be working when i change that to use selectionManager

adobe-bot · 2022-05-10T01:18:38Z

Build successful! 🎉

reidbarber

LGTM, new styles render as expected

…spectrum into sidenav-styles-for-listview

adobe-bot · 2022-05-10T21:08:23Z

Build successful! 🎉

adobe-bot · 2022-05-10T21:09:45Z

Build successful! 🎉

LFDanLu

LGTM, I'll approve if you think we can ignore that wiggle :D

LFDanLu · 2022-05-10T21:21:24Z

packages/@react-spectrum/list/src/styles.css

-      /* Box shadow for bottom border for non-selected rows that is immmediately above a selected row. */
-      &.is-next-selected {
-        &:after {
-          box-shadow: inset 0 -1px 0 0 var(--spectrum-global-color-blue-500);
-        }
-      }


Minor note, potentially ignorable:
By simplifying this, a wiggle case has returned:

Go to https://reactspectrum.blob.core.windows.net/reactspectrum/59530d349cab3e68b3c57100957b6659c7effe87/storybook/index.html?path=/story/listview--selection-multiple-checkbox and select the second row.

Repeatedly select and deselect the first row. Note that the border between the two rows wiggles

I'm fine with ignoring it in favor of simpler css, I probably overengineered this css to avoid this wiggle in the first place haha

Ok, I'll have a look at this separately. good find though

devongovett

Approving in case you want to look at Daniel's wiggle separately

Round ListView item borders in single selection

40ba903

snowystinger added 2 commits May 5, 2022 11:19

support multiple selection

06aaf58

Merge branch 'main' into sidenav-styles-for-listview

18baa67

snowystinger changed the title ~~Round ListView item borders in single selection~~ Round ListView item borders in quiet May 5, 2022

LFDanLu reviewed May 6, 2022

View reviewed changes

devongovett reviewed May 7, 2022

View reviewed changes

Merge branch 'main' into sidenav-styles-for-listview

474e77c

# Conflicts: # packages/@react-spectrum/list/src/ListView.tsx # packages/@react-spectrum/list/src/styles.css

Move css logic to js

efcaf6c

devongovett reviewed May 9, 2022

View reviewed changes

Merge branch 'main' into sidenav-styles-for-listview

7b1a6e3

Merge branch 'main' into sidenav-styles-for-listview

cddd90b

Remove hover edge joining

f39193e

snowystinger added 2 commits May 9, 2022 15:42

fix rounding logic for focus leaving

540adf1

remove console.log

74e7ba7

devongovett reviewed May 9, 2022

View reviewed changes

Fix hover

504babb

reidbarber previously approved these changes May 10, 2022

View reviewed changes

LFDanLu and others added 3 commits May 10, 2022 14:01

Merge branch 'main' into sidenav-styles-for-listview

c6133b2

rely on selection manager isFocused for focuswithin

12cfa98

Merge branch 'sidenav-styles-for-listview' of github.com:adobe/react-…

59530d3

…spectrum into sidenav-styles-for-listview

snowystinger dismissed reidbarber’s stale review via 59530d3 May 10, 2022 21:02

reidbarber approved these changes May 10, 2022

View reviewed changes

LFDanLu reviewed May 10, 2022

View reviewed changes

devongovett approved these changes May 10, 2022

View reviewed changes

snowystinger merged commit 01fafa7 into main May 10, 2022

snowystinger deleted the sidenav-styles-for-listview branch May 10, 2022 22:24

LFDanLu added the needs testing label May 11, 2022

dannify removed the needs testing label Jun 3, 2022

Round ListView item borders in quiet #3105

Round ListView item borders in quiet #3105

Uh oh!

Conversation

snowystinger commented May 5, 2022

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

snowystinger commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adobe-bot commented May 5, 2022

Uh oh!

adobe-bot commented May 5, 2022

Uh oh!

majornista commented May 5, 2022

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett commented May 7, 2022

Uh oh!

adobe-bot commented May 7, 2022

Uh oh!

adobe-bot commented May 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowystinger May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented May 9, 2022

Uh oh!

adobe-bot commented May 9, 2022

Uh oh!

adobe-bot commented May 9, 2022

Uh oh!

adobe-bot commented May 9, 2022

Uh oh!

adobe-bot commented May 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented May 10, 2022

Uh oh!

reidbarber left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented May 10, 2022

Uh oh!

adobe-bot commented May 10, 2022

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

snowystinger commented May 5, 2022 •

edited

Loading

snowystinger May 9, 2022 •

edited

Loading