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

feat: Make Breadcrumb group component responsive #2671

Merged
merged 7 commits into from
Sep 12, 2024

Conversation

fralongo
Copy link
Member

@fralongo fralongo commented Sep 5, 2024

Description

  • Breadcrumbs adjust to the available space, as opposed to the viewport size.
  • they will always occupy one line
  • items that are shorter than a MIN_WIDTH will not shrink and truncate
  • items that longer than a MIN_WIDTH will shrink and truncate until they reach the MIN_WIDTH.
    • The longer an item, the faster it will shrink. This creates a balance in the length of truncated items
  • When all items have reached the MIN_WIDTH, some of the items will be collapsed into a dropdown.
    • the collapsing behavior will start with the second item on the left (in LTR applications), to prioritize immediate visibility for the last items, that are supposed to be closer in context to the current page
  • When the last item is truncated it becomes focusable to allow for the display of the tooltip with keyboard
Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates. N/A
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md. N/A
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests? Yes
  • Changes are covered with new/existing integration tests? Yes

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@fralongo fralongo requested a review from a team as a code owner September 5, 2024 15:23
@fralongo fralongo requested review from johannes-weber and removed request for a team September 5, 2024 15:23
@fralongo fralongo changed the title Flongo responsive breadcrumbs feat: Make Breadcrumb group component responsive Sep 5, 2024
@fralongo fralongo force-pushed the flongo-responsive-breadcrumbs branch from 7e272da to 50c1f00 Compare September 5, 2024 15:26
@fralongo fralongo requested review from connorlanigan and removed request for johannes-weber September 6, 2024 08:54
Copy link

codecov bot commented Sep 6, 2024

Codecov Report

Attention: Patch coverage is 99.10714% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.92%. Comparing base (f014c5b) to head (f37ceed).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
src/breadcrumb-group/item/item.tsx 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2671      +/-   ##
==========================================
+ Coverage   95.79%   95.92%   +0.12%     
==========================================
  Files         744      745       +1     
  Lines       20569    20658      +89     
  Branches     6964     6663     -301     
==========================================
+ Hits        19705    19817     +112     
- Misses        808      833      +25     
+ Partials       56        8      -48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/breadcrumb-group/__integ__/breadcrumb-group.test.ts Outdated Show resolved Hide resolved
src/breadcrumb-group/__integ__/breadcrumb-group.test.ts Outdated Show resolved Hide resolved
src/breadcrumb-group/__integ__/breadcrumb-group.test.ts Outdated Show resolved Hide resolved
src/breadcrumb-group/implementation.tsx Outdated Show resolved Hide resolved
src/breadcrumb-group/implementation.tsx Outdated Show resolved Hide resolved
connorlanigan
connorlanigan previously approved these changes Sep 10, 2024
@connorlanigan connorlanigan self-requested a review September 11, 2024 16:32
connorlanigan
connorlanigan previously approved these changes Sep 11, 2024
@fralongo fralongo added this pull request to the merge queue Sep 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 12, 2024
@fralongo fralongo added this pull request to the merge queue Sep 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 12, 2024
@fralongo fralongo added this pull request to the merge queue Sep 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Sep 12, 2024
@fralongo fralongo added this pull request to the merge queue Sep 12, 2024
Merged via the queue into main with commit d9a5f98 Sep 12, 2024
33 checks passed
@fralongo fralongo deleted the flongo-responsive-breadcrumbs branch September 12, 2024 14:12
setItemsWidths(newItemsWidths);
}
}
}, [itemsWidths, items, navWidth]);
Copy link
Member

Choose a reason for hiding this comment

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

You have a double render loop here. After calling setItemsWidth, this effect is called second time, because it tracks itemsWidths value which got just changed.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested solution

setItemsWidths(oldWidths => {
  if (
    !areArrayEqual(newItemsWidths.ghost, oldWidths.ghost) ||
    !areArrayEqual(newItemsWidths.real, oldWidths.real)
  ) {
    return newItemsWidths;
  } else {
    return oldWidths
  }
})

@@ -106,10 +126,45 @@ export function BreadcrumbGroupImplementation<T extends BreadcrumbGroupProps.Ite
checkSafeUrl('BreadcrumbGroup', item.href);
}
const baseProps = getBaseProps(props);
const isMobile = useMobile();
const [navWidth, navRef] = useContainerQuery<number>(rect => rect.contentBoxWidth);
Copy link
Member

Choose a reason for hiding this comment

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

This should be rect.borderBoxWidth to incorporate inner paddings, if there are any

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants