RSP-1387|RSP-1393: Improve Breadcrumbs auto collapse behavior #148

pst67662 · 2019-12-25T14:57:52Z

https://jira.corp.adobe.com/browse/RSP-1387
When there isn't enough space, the breadcrumbs shouldn't immediately collapse into a single
dropdown but move one by one into the dropdown until it fits.

Stories:

https://jira.corp.adobe.com/browse/RSP-1393
The collapse behavior should work with the multi-line variant

Story:

maxVisibleItems: 'auto', size: 'L'

NOTE: needs design review (should last/selected item be visible?)

✅ Pull Request Checklist:

Included link to corresponding 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 exist for this component).
Looked at the Accessibility Standard and/or talked to @mijordan about Accessibility for this feature.

📝 Test Instructions:

🧢 Your Team:

adobe-bot · 2019-12-25T15:01:40Z

Build successful! View the storybook

adobe-bot · 2019-12-26T12:44:30Z

Build successful! View the storybook

snowystinger · 2020-01-10T21:36:44Z

packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx

  let domRef = useDOMRef(ref);
+  let lsitRef = useRef(null);


I think you meant listRef?

snowystinger · 2020-01-10T22:20:30Z

packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx

-        setHidden(domRef.current.scrollWidth > domRef.current.offsetWidth);
+      if (isCollapsible && lsitRef.current) {
+        let containterRect = lsitRef.current.getBoundingClientRect();
+        let index = childrenRect.findIndex(childRect => childRect.right > containterRect.right);


you'll need to loop over the childrenRect's during onResize to get their Bounding Client Rect's.
check out the storybook for this, you'll see that items disappear from the list much too quickly.
the reason for this is that the 'right' value for each of the items is whatever they started at, then, as the window shrinks, the left and right of the containing ul move to the left because it's centered and 50vw.

we should add some tests for this as well

snowystinger · 2020-01-10T22:27:49Z

packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx

    ...otherProps
  } = props;
  let {styleProps} = useStyleProps(otherProps);

  let isCollapsible = maxVisibleItems === 'auto';
  let childArray = React.Children.toArray(children);

-  const [hidden, setHidden] = useState(false);
+  const [visibleItems, setVisibleItems] = useState(isCollapsible ? childArray.length : maxVisibleItems);


this state should probably be in stately
calculating bounding rects should stay in this file

and this should probably be over in stately as well along with an rtl control

let [visibleItems, setVisibleItems] = useState(isCollapsible ? childArray.length : maxVisibleItems); function setState(childrenRects, containerRect, direction='rtl') { let index = childrenRects.findIndex(childRect => direction === 'rtl' ? childRect.right > containerRect.right : childRect.left < containerRect.left); let visibleItemsCount; let minVisibleItems = showRoot ? MIN_VISIBLE_ITEMS + 1 : MIN_VISIBLE_ITEMS; if (index === -1) { visibleItemsCount = childArray.length; } else if (index < minVisibleItems) { visibleItemsCount = minVisibleItems; } else { visibleItemsCount = index; } setVisibleItems(visibleItemsCount); } return [visibleItems, setState];

something along those lines anyways

adobe-bot · 2020-01-17T15:38:32Z

Build successful! View the storybook

adobe-bot · 2020-01-21T13:15:39Z

Build successful! View the storybook

adobe-bot · 2020-01-21T15:01:30Z

Build successful! View the storybook

adobe-bot · 2020-01-21T18:52:38Z

Build successful! View the storybook

snowystinger · 2020-01-21T18:54:44Z

packages/@react-spectrum/breadcrumbs/src/Breadcrumbs.tsx

-      window.removeEventListener('resize', onResize);
-    };
-  }, [domRef, isCollapsible]);
+  let {visibleItems} = useBreadcrumbsState(props, listRef);


It's tempting to just pass the listRef in, however, we really need to keep anything DOM associated out of the the stately hooks, otherwise we can't use them in environments (ie native/uxp)
Lets pass the values that we'll use to calculate the state and not the whole dom ref

snowystinger · 2020-01-21T19:00:00Z

packages/@react-stately/breadcrumbs/src/useBreadcrumbsState.ts

+      }
+    };
+
+    window.addEventListener('resize', onResize);


resize should stay out in @react-spectrum instead of being stately because it's specific to web
please refer back to my original comment for what should be moved from @react-spectrumt to stately, it shouldn't be a lot of code, really just the state and maybe a minimal amount of logic to dictate how collapsing is calculated

I think @react-aria is probably the right place for this useEffect block entirely.

@devongovett can you explain more? I get that react-aria is for interactions, but i thought that was for direct interactions, not indirect ones like window resize.
I'm happy to have this put into react-aria, just want some clarification :)

@snowystinger I moved useEffect block to react-aria (and left minimal state logic here), but that means passing listRef to useBreadcrumbs and doing dom related calculations there... please check if thats OK

snowystinger · 2020-01-24T18:07:36Z

packages/@react-stately/breadcrumbs/src/useBreadcrumbsState.ts

+  const [visibleItems, setVisibleItems] = useState(isCollapsible ? childArray.length : maxVisibleItems);
+
+  useEffect(() => {
+    let listItems = Array.from(ref.current.children);


I wonder if it's better to do ref.current.children
or for each breadcrumb to expose via useImperativeHandle their own width
thoughts for later, i'm fine with doing it this way right now

like the idea, but I would definitely need some help with useImperativeHandle... is it OK if I address this in additional task?

adobe-bot · 2020-01-29T19:06:59Z

Build successful! View the storybook

adobe-bot · 2020-01-31T12:48:27Z

Build successful! View the storybook

snowystinger

Looks really good! thank you!
Last question I promise, the dropdown, it should contain every item regardless of its visibility in the breadcrumbs that aren't collapsed?

LFDanLu

LGTM

adobe-bot · 2020-02-01T01:41:27Z

Build successful! View the storybook

pst67662 · 2020-02-03T14:40:16Z

@snowystinger from XD guidelines it seems like the dropdown contains every item, which is kinda confusing... maybe to advice with the design team? (that there is an option to only show collapsed items in a dropdown)

* initial Breadcrumbs work * improve link styles * simplify styles * add flex to macro * fix multiline * more style improvements * allow more link props * pass down link props * remove s2 link context and styles added * remove underline for disabled link * review follow-up * use marginStart: text-to-visual for chevron * remove design link * remove shorthand flex from theme * remove nav wrapper * update styles * spread props onto RAC breadcrumbs * add onAction story * add styles to Heading * ad Heading story to Breadcrumbs * fixing breadcrumb vertical padding when multiline only had to change the height, the gap between the top and current breadcrumb is already provided by the row gap of 4 which equals the breadcrumbs-start-edge-to-truncated-menu value in the spec. This value is also moreaccurate than the 9px they have for top-text-to-to-bottom-text since it starts at the bottom of the breadcrumb box * fix active style, chevron centering, and tentative styles for multiline see comments for uncertainties with margins and multiline in design * update default, wrapper padding, and isMultiLine sizing as per design feedback * review comments * fix Safari focus ring cut off * get rid of default heading styling for now --------- Co-authored-by: Reid Barber <reid@reidbarber.com> Co-authored-by: danilu <danilu@adobe.com>

RSP-1387: Improve Breadcrumbs auto collapse behavior

74a9277

pst67662 requested review from dannify, devongovett and snowystinger as code owners December 25, 2019 14:57

RSP-1387: adding collapsed story

f1009d0

pst67662 changed the title ~~RSP-1387: Improve Breadcrumbs auto collapse behavior~~ RSP-1387|RSP-1393: Improve Breadcrumbs auto collapse behavior Dec 30, 2019

snowystinger reviewed Jan 10, 2020

View reviewed changes

pst67662 added 2 commits January 17, 2020 16:25

RSP-1387 - add useBreadcrumbsState

aa5016a

RSP-1387 - package version

9a67ed2

RSP-1387: replace rect calc

be90a9e

RSP-1387: adding tests

4f81b88

Merge branch 'master' into RSP-1387

094e8c6

snowystinger reviewed Jan 21, 2020

View reviewed changes

snowystinger reviewed Jan 24, 2020

View reviewed changes

RSP-1387: moving useEffect block to react-aria

5e14bb8

RSP-1387: useEffect block and useState to react-spectrum

c06e1f5

snowystinger approved these changes Jan 31, 2020

View reviewed changes

LFDanLu approved these changes Feb 1, 2020

View reviewed changes

Merge branch 'master' into RSP-1387

3f7fc65

dannify merged commit b473e14 into master Feb 1, 2020

dannify deleted the RSP-1387 branch February 1, 2020 01:41

aravindb26 mentioned this pull request Mar 3, 2024

[Snyk] Upgrade tailwind-variants from 0.1.20 to 0.2.0 aravindb26/react-spectrum#3

Open

RSP-1387|RSP-1393: Improve Breadcrumbs auto collapse behavior #148

RSP-1387|RSP-1393: Improve Breadcrumbs auto collapse behavior #148

Uh oh!

Conversation

pst67662 commented Dec 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Team:

Uh oh!

adobe-bot commented Dec 25, 2019

Uh oh!

adobe-bot commented Dec 26, 2019

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!

adobe-bot commented Jan 17, 2020

Uh oh!

adobe-bot commented Jan 21, 2020

Uh oh!

adobe-bot commented Jan 21, 2020

Uh oh!

adobe-bot commented Jan 21, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Jan 29, 2020

Uh oh!

adobe-bot commented Jan 31, 2020

Uh oh!

snowystinger left a comment

Choose a reason for hiding this comment

Uh oh!

LFDanLu left a comment

Choose a reason for hiding this comment

Uh oh!

adobe-bot commented Feb 1, 2020

Uh oh!

pst67662 commented Feb 3, 2020

Uh oh!

Uh oh!

pst67662 commented Dec 25, 2019 •

edited

Loading