feat(listitembase): dynamic list support #600

Coread · 2024-08-30T16:14:42Z

Description

This PR contains a number of improvements to List/ListItemBase:

Improved handling of adding and removing items from the list
Exclusion of single tabbable elements from the focus handling logic in ListItemBase
onFocus/blur and onFocusWithin/blurWithin

Links

Links to relevent resources.

martroos · 2024-09-02T09:17:36Z

src/components/ListItemBase/ListItemBase.tsx

@@ -271,6 +271,24 @@ const ListItemBase = (props: Props, providedRef: RefOrCallbackRef) => {
    };
  }, []);

+  useEffect(() => {


issue: This breaks looping lists if one of the ends is considered empty - maybe not a huge issue, but it doesn't behave as expected.

this is now fixed

martroos · 2024-09-02T09:18:29Z

src/components/ListItemBase/ListItemBase.tsx

+        }
+      }
+    }
+  }, [focus, lastCurrentFocus, listContext.listSize]);


suggestion: I think this should include indexItem and probably listContext.setCurrentFocus (the latter is admittedly the result of a useState so not hugely important, but this module doesn't really know what it is based on the context).

I've added the full list of dependencies to the useEffect

martroos · 2024-09-02T09:20:13Z

src/components/ListItemBase/ListItemBase.tsx

+      if (lastCurrentFocus > itemIndex) {
+        if (itemIndex <= 0) {
+          listContext.setCurrentFocus(itemIndex + 1);
+        } else {
+          listContext.setCurrentFocus(itemIndex - 1);
+        }
+      } else {
+        if (itemIndex >= listContext.listSize - 1) {
+          listContext.setCurrentFocus(itemIndex - 1);
+        } else {
+          listContext.setCurrentFocus(itemIndex + 1);
+        }


suggestion: Some comments to explain what is happening would be useful - with some analysis you can see that you are actively keeping items at the (direction dependent) previous item if this item is at one of the ends, and otherwise skipping it, but it's not necessarily simple at a glance (the comment would have also made it clear that looping wouldn't work any more).

this is now fixed

martroos · 2024-09-02T09:21:16Z

src/components/List/List.stories.tsx

+const ListWithNullElementsWrapper = () => {
+  return (
+    <>
+      <List noLoop listSize={7}>


issue: This is a list of length 9 (looping will happen to work going down because this number is wrong!).

martroos · 2024-09-02T09:26:23Z

src/components/ListItemBase/ListItemBase.style.scss

+    clip: rect(0 0 0 0);
+    clip-path: inset(50%);
+    height: 1px;
+    overflow: hidden;
+    position: absolute;
+    width: 1px;
+    white-space: nowrap;


nitpick: This is another instance of the "visually hidden" styling which is in various parts of the library, would be nice to mixin / include this in scss terms.

as far as I know, we don't currently have shared scss between the new components?

… into allow_empty_list_items

maxinteger · 2024-09-10T09:08:01Z

src/components/ListItemBase/ListItemBase.types.ts

+  /**
+   * event fired when any of the children or the element itself are focused
+   */
+  onFocusWithin?: (e: React.FocusEvent<Element>) => void;
+
+  /**
+   * event fired when any of the children or the element itself lose focus
+   */
+  onBlurWithin?: (e: React.FocusEvent<Element>) => void;
+  /**
+   * event fired when any of the children or the element itself are focused
+   */
+  onFocus?: (e: React.FocusEvent<Element>) => void;
+
+  /**
+   * event fired when any of the children or the element itself lose focus
+   */
+  onBlur?: (e: React.FocusEvent<Element>) => void;


Props should extends FocusProps and FocusWithinProps instead of manually adding the props. JSDocs should be moved to the original definition.

I'm now extending this definition using the props from react-aria. Note that it isn't as simple as extending directly, because I haven't added support for all of FocusProps or all of FocusWithinProps

maxinteger · 2024-09-10T09:14:04Z

src/components/List/List.stories.tsx

+  return (
+    <>
+      <List shouldFocusOnPress listSize={2}>
+        <ListItemBase data-exclude-focus-children size="auto" itemIndex={0} key={0}>


What is the expected behaviour for data-exclude-focus and data-exclude-focus-children? I can see they might be excluded from intractable element selection, but it is not prevent the browser focus the element with Tab.

I check this story, and I can focus on button 1 with Tab.

I've removed the exclude-focus-children as this was covered by your recent changes. data-exclude-focus is used by the Aria-Toolbar. The toolbar only wants to expose one of its tabable buttons to preserve focus order when you tab forward/backward from within a ListItemBase. It sets data-exclude-focus on all but the first one so the first element is the only one to ever be in the tab order.

maxinteger · 2024-09-10T09:34:19Z

src/hooks/useFocusState.ts

+      if (props.onFocusWithin) {
+        props.onFocusWithin.apply(this, args);
+      }


nitpicking: this can be simplified to:

Suggested change

if (props.onFocusWithin) {

props.onFocusWithin.apply(this, args);

}

props.onFocusWithin?.(...args);

because the arrow functions this is undefined anyway

I've changed this

…omentum-react-v2 into allow_empty_list_items

… into allow_empty_list_items

Stale review

…items

.eslintrc.js

… into allow_empty_list_items

jor-row

Tested locally and changes look good to me. A few small comments

jor-row · 2024-10-02T12:11:21Z

src/components/List/List.stories.tsx

+const ListWithNonFocusableChildrenWrapper = () => {
+  return (
+    <>
+      <List shouldFocusOnPress listSize={2}>
+        <ListItemBase data-preserve-tabindex size="auto" itemIndex={0} key={0}>
+          <ButtonPill>1</ButtonPill>
+        </ListItemBase>
+        <ListItemBase itemIndex={1} key={1}>
+          <ButtonPill>2</ButtonPill>
+        </ListItemBase>
+      </List>
+    </>
+  );
+};
+const ListWithNonFocusableChildren = Template<unknown>(ListWithNonFocusableChildrenWrapper).bind(
+  {}
+);


This story doesn't seem to be working as expected

jor-row · 2024-10-02T12:14:09Z

src/components/AriaToolbar/AriaToolbar.tsx

    }
  };

+  useEffect(() => {
+    // When the toolbar is rendered inside a list, only item in the toolbar


"only one item" or "only the first item"

jor-row · 2024-10-02T13:04:41Z

src/hooks/useOrientationBasedKeyboardNavigation.ts

  getContext: () => {
    listSize: number;
    currentFocus: number;
    setCurrentFocus: Dispatch<SetStateAction<number>>;
    shouldFocusOnPress?: boolean;
    shouldItemFocusBeInset?: boolean;
+    noLoop?: boolean;
+    updateFocusBlocked?: boolean;
+    supressFocus?: boolean;


supressFocus is never used so can be removed

feat(listitembase): allow empty items

bd6d2e0

martroos previously requested changes Sep 2, 2024

View reviewed changes

Coread added 4 commits September 2, 2024 16:05

feat(list): tests for list item base

a456235

feat(listitembase): prevent initial autofocus

8a7be16

test(aria-toolbar): update snapshot

3c0f5d7

feat(list-item): add full list of dependencies to useEffect

e6ad993

Coread marked this pull request as ready for review September 3, 2024 08:16

Coread added 9 commits September 3, 2024 12:33

feat(list-item): make empty elements aria-hidden

0063601

feat(list-item): focusChild

34698aa

feat(list): remove unused import

7d6fe38

Merge branch 'master' of github.com:momentum-design/momentum-react-v2…

2a14e38

… into allow_empty_list_items

feat(listitem): initial focus

9574861

feat(listitem): remove unused import

11ea311

Merge branch 'master' of github.com:momentum-design/momentum-react-v2…

b0b9218

… into allow_empty_list_items

feat(listitem): focus within handling

2fd6364

feat(list): don't update initialFocus while focused

eabcb5f

maxinteger requested changes Sep 10, 2024

View reviewed changes

maxinteger reviewed Sep 10, 2024

View reviewed changes

Coread and others added 12 commits September 12, 2024 11:07

feat(list): dynamic list update

70e7105

feat(listitem): fix text selection and press events

9dc5a80

fix(cursor): text selection

cdf6616

fix(meeting-list-item): update tests and stories

ce97696

Merge branch 'allow_empty_list_items' of github.com:momentum-design/m…

779ac9f

…omentum-react-v2 into allow_empty_list_items

feat(listitembase): remove handling for empty list items

57721d2

feat(listitembase): change to updateFocusBlocked

e8cbc5f

Merge branch 'master' of github.com:momentum-design/momentum-react-v2…

802a71f

… into allow_empty_list_items

feat(list-item): remove allowExclusions

7596254

fix(list): supress incorrect lint warning

04d48fc

feat(hooks): simplify focus hooks

bef0141

feat(listitembase): extend types instead of redefining

16a39b8

Coread changed the title ~~feat(listitembase): allow empty items~~ feat(listitembase): dynamic list support Sep 22, 2024

jor-row added 4 commits September 26, 2024 14:01

chore: merge latest master

19bbbb3

fix(tests): unit and snapshots

16be115

Merge remote-tracking branch 'upstream/master' into allow_empty_list_…

6b46bd8

…items

fix(tree): unit tests

3f94da1

jor-row requested changes Sep 30, 2024

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

Coread added 4 commits October 1, 2024 13:04

fix(tree): ignore deps in useEffects

bf44346

fix(menu): ignore exhaustive deps

b89da7c

Merge branch 'master' of github.com:momentum-design/momentum-react-v2…

dae3c7b

… into allow_empty_list_items

feat(button-group): fix type

5de7adf

jor-row approved these changes Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(listitembase): dynamic list support #600

feat(listitembase): dynamic list support #600

Coread commented Aug 30, 2024 •

edited

Loading

martroos Sep 2, 2024

Coread Sep 3, 2024

martroos Sep 2, 2024

Coread Sep 3, 2024

martroos Sep 2, 2024

Coread Sep 3, 2024

martroos Sep 2, 2024

Coread Sep 3, 2024

martroos Sep 2, 2024

Coread Sep 3, 2024

maxinteger Sep 10, 2024

Coread Sep 22, 2024

maxinteger Sep 10, 2024

Coread Sep 22, 2024

maxinteger Sep 10, 2024

Coread Sep 22, 2024

jor-row left a comment

jor-row Oct 2, 2024

jor-row Oct 2, 2024

jor-row Oct 2, 2024

feat(listitembase): dynamic list support #600

Are you sure you want to change the base?

feat(listitembase): dynamic list support #600

Conversation

Coread commented Aug 30, 2024 • edited Loading

Description

Links

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jor-row left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coread commented Aug 30, 2024 •

edited

Loading