Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breadcrumb collapse #6892

Merged
merged 26 commits into from
Aug 30, 2024
Merged

Breadcrumb collapse #6892

merged 26 commits into from
Aug 30, 2024

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Aug 16, 2024

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:

@rspbot
Copy link

rspbot commented Aug 16, 2024

@rspbot
Copy link

rspbot commented Aug 19, 2024

@rspbot
Copy link

rspbot commented Aug 19, 2024

@rspbot
Copy link

rspbot commented Aug 20, 2024

@rspbot
Copy link

rspbot commented Aug 21, 2024

@rspbot
Copy link

rspbot commented Aug 21, 2024

# Conflicts:
#	packages/@react-spectrum/s2/src/Breadcrumbs.tsx
#	packages/@react-spectrum/s2/src/TagGroup.tsx
@snowystinger snowystinger marked this pull request as ready for review August 21, 2024 06:15
@rspbot
Copy link

rspbot commented Aug 21, 2024

@rspbot
Copy link

rspbot commented Aug 21, 2024

@rspbot
Copy link

rspbot commented Aug 21, 2024

@rspbot
Copy link

rspbot commented Aug 22, 2024

@rspbot
Copy link

rspbot commented Aug 22, 2024

packages/@react-spectrum/s2/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
packages/@react-spectrum/s2/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior looks good to me, just a couple of questions

packages/@react-spectrum/s2/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
<HiddenBreadcrumbs items={children} size={size} listRef={listRef} />
{visibleItems < collection.size ? (
<>
{children[0].render?.(children[0])}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to keep the showRoot option from v3?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following up with spectrum

packages/@react-spectrum/s2/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
@rspbot
Copy link

rspbot commented Aug 29, 2024

@rspbot
Copy link

rspbot commented Aug 29, 2024

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing I noticed

packages/react-aria-components/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
{visibleItems < collection.size ? (
<>
{children[0].render?.(children[0])}
<BreadcrumbMenu items={children.slice(1, sliceIndex)} onAction={onAction} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you only have two breadcrumbs? I assume it shouldn't show the breadcrumb menu still? It seems if you collapse it enough when there are only two items that the Breadcrumb menu shows up:

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, see storybook from ac304c9 where i temporarily added some new stories to hit the edge cases

@rspbot
Copy link

rspbot commented Aug 29, 2024

@rspbot
Copy link

rspbot commented Aug 29, 2024

@rspbot
Copy link

rspbot commented Aug 29, 2024

LFDanLu
LFDanLu previously approved these changes Aug 29, 2024
Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified the link out arrows aren't there anymore in the stories, breadcrumb text and chevron disabled color and the 2 breadcrumb item case

reidbarber
reidbarber previously approved these changes Aug 29, 2024
packages/@react-spectrum/s2/src/Breadcrumbs.tsx Outdated Show resolved Hide resolved
@rspbot
Copy link

rspbot commented Aug 29, 2024

Co-authored-by: Reid Barber <reid@reidbarber.com>
@snowystinger snowystinger dismissed stale reviews from reidbarber and LFDanLu via 96d3cb5 August 29, 2024 23:22
@rspbot
Copy link

rspbot commented Aug 29, 2024

@rspbot
Copy link

rspbot commented Aug 29, 2024

## API Changes

react-aria-components

/react-aria-components:BreadcrumbRenderProps

 BreadcrumbRenderProps {
   isCurrent: boolean
+  isDisabled: boolean
 }

@snowystinger snowystinger merged commit 553ef94 into main Aug 30, 2024
29 checks passed
@snowystinger snowystinger deleted the breadcrumb-menu branch August 30, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants