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.

Sign up for GitHub

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

Jump to bottom

fix(mui): Fixed condition preventing Breadcrumb text display #6503

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
+44 −18
Open

fix(mui): Fixed condition preventing Breadcrumb text display #6503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
aa3c7ae
fix: fix a bug whereby breadcrumbs are not shown
aress31 Nov 18, 2024
feb06eb
fix: fix a bug whereby breadcrumbs are not shown #1
aress31 Nov 18, 2024
4925b2b
chore: added changesets
aress31 Nov 18, 2024
9db34df
chore: style cleanup
aress31 Nov 18, 2024
f9063ce
refactor(mui): refactored breadcrumbs, added modularity and harmonise…
aress31 Nov 18, 2024
1236d0a
refactor: removed superflous padding
aress31 Nov 18, 2024
bfe1f33
refactor: focus on changes
aress31 Nov 19, 2024
1bfd850
refactor: applying suggested changes
aress31 Nov 20, 2024
babfe39
fix: one failed test
aress31 Nov 20, 2024
9cb9aa9
fix: fix rendering condition
aress31 Nov 20, 2024
101ea4a
chore: remove unnecessary changeset
alicanerdurmaz Nov 21, 2024
8702671
chore: add issue link to changeset
alicanerdurmaz Nov 21, 2024
f09a88b
fix: https://github.com/refinedev/refine/pull/6503#discussion_r185155…
aress31 Nov 21, 2024
99dc85d
Merge branch '6497' of https://github.com/aress31/refine into 6497
aress31 Nov 21, 2024
f952ed3
fix: went for more logical option
aress31 Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .changeset/sweet-brooms-wonder.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
"@refinedev/chakra-ui": minor
"@refinedev/ui-tests": minor
"@refinedev/ui-types": minor
"@refinedev/mantine": minor
"@refinedev/antd": minor
"@refinedev/mui": minor
---

feat: added `minItems` prop to specify the minimum number of items required for rendering breadcrumbs. #6497


Resolves [#6497](https://github.com/refinedev/refine/issues/6497)
5 changes: 2 additions & 3 deletions packages/antd/src/components/breadcrumb/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({
showHome = true,
hideIcons = false,
meta,
minItems = 2,
}) => {
const routerType = useRouterType();
const { breadcrumbs } = useBreadcrumb({
Expand All @@ -38,9 +39,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({

const ActiveLink = routerType === "legacy" ? LegacyLink : Link;

if (breadcrumbs.length === 1) {
return null;
}
if (breadcrumbs.length < minItems) return null;
Copy link
Member

Choose a reason for hiding this comment

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

  • case 1: breadcrumbs.length = 0: // both versions return null
  • case 2: breadcrumbs.length = 1: // old version returns null, new version shows
  • case 3: breadcrumbs.length = 2: // Both versions renders

we have two options to fix case 2:
1.

Suggested change
if (breadcrumbs.length < minItems) return null;
if (breadcrumbs.length <= minItems) return null;
  1. default should be set to 2 minItems = 2

I believe second option is better because it fits minItems name.

Copy link
Author

@aress31 aress31 Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why should the default be set to minItems = 2 by default?

What’s the rationale behind this choice? If a route is configured, the breadcrumb should display it by default, unless specified otherwise (and that is the point of this new minItems prop).

Regardless I pushed suggested changes, if you change your mind, please feel free to discard the commit. :)

Copy link
Member

Choose a reason for hiding this comment

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

Because previously, the breadcrumb would render only if there were at least 2 items, but it wouldn’t render if there was only 1 item. So, minItems should be 2. If we break this logic, it would be a breaking change, and current users who update Refine would see a breadcrumb even in cases with just 1 item.

I'm with you. If the route is configured, it should render, but we shouldn't make breaking changes, even if the old implementation is wrong.

Copy link
Author

Choose a reason for hiding this comment

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

We should be good to merge then since I have the default for minItems to be 2. Please let me know if there is another blocker.

Also for future major release might be a good idea to implement the breaking change of setting minItems to 1.


const breadCrumbItems = breadcrumbs.map(({ label, icon, href }) => ({
key: `breadcrumb-item-${label}`,
Expand Down
5 changes: 2 additions & 3 deletions packages/chakra-ui/src/components/breadcrumb/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({
showHome = true,
hideIcons = false,
meta,
minItems = 2,
}) => {
const routerType = useRouterType();
const { breadcrumbs } = useBreadcrumb({ meta });
Expand All @@ -33,9 +34,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({

const ActiveLink = routerType === "legacy" ? LegacyLink : Link;

if (breadcrumbs.length === 1) {
return null;
}
if (breadcrumbs.length < minItems) return null;

const { resources } = useResource();

Expand Down
5 changes: 2 additions & 3 deletions packages/mantine/src/components/breadcrumb/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({
showHome = true,
hideIcons = false,
meta,
minItems = 2,
}) => {
const routerType = useRouterType();
const { breadcrumbs } = useBreadcrumb({ meta });
Expand All @@ -40,9 +41,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({

const ActiveLink = routerType === "legacy" ? LegacyLink : Link;

if (breadcrumbs.length === 1) {
return null;
}
if (breadcrumbs.length < minItems) return null;

return (
<Breadcrumbs
Expand Down
5 changes: 2 additions & 3 deletions packages/mui/src/components/breadcrumb/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({
showHome = true,
hideIcons = false,
meta,
minItems = 2,
}) => {
const { breadcrumbs } = useBreadcrumb({ meta });
const routerType = useRouterType();
Expand All @@ -41,9 +42,7 @@ export const Breadcrumb: React.FC<BreadcrumbProps> = ({

const rootRouteResource = matchResourceFromRoute("/", resources);

if (breadcrumbs.length === 1) {
return null;
}
if (breadcrumbs.length < minItems) return null;

const LinkRouter = (props: LinkProps & { to?: string }) => (
<Link {...props} component={ActiveLink} />
Expand Down
20 changes: 14 additions & 6 deletions packages/ui-tests/src/tests/breadcrumb.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,28 @@ export const breadcrumbTests = (
Breadcrumb: React.ComponentType<RefineBreadcrumbProps<any>>,
): void => {
describe("[@refinedev/ui-tests] Common Tests / CRUD Create", () => {
it("should render successfuly", async () => {
it("should not render breadcrumb when no items are present", async () => {
const { container } = renderBreadcrumb(<Breadcrumb />);

expect(container).toBeTruthy();
expect(container).toBeEmptyDOMElement();
});

it("should render breadcrumb items", async () => {
const { getByText } = renderBreadcrumb(<Breadcrumb />, {
it("should render breadcrumb when the number of items is equal to or greater than minItems", async () => {
const { getByText } = renderBreadcrumb(<Breadcrumb minItems={1} />, {
resources: [{ name: "posts" }],
routerInitialEntries: ["/posts/create"],
routerInitialEntries: ["/posts"],
});

getByText("Posts");
getByText("Create");
});

it("should not render breadcrumb when the number of items is less than minItems", async () => {
const { container } = renderBreadcrumb(<Breadcrumb minItems={3} />, {
resources: [{ name: "posts" }],
routerInitialEntries: ["/posts/create"],
});

expect(container).toBeEmptyDOMElement();
});

it("should render breadcrumb items with link", async () => {
Expand Down
9 changes: 9 additions & 0 deletions packages/ui-types/src/types/breadcrumb.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,25 @@ export type RefineBreadcrumbProps<
* Passes properties for <Breadcrumb>
*/
breadcrumbProps?: TBreadcrumbProps;

/**
* Shows the home button if you have a resource with `list` action defined as `/` route.
*/
showHome?: boolean;

/**
* Allows to hide resource icons
*/
hideIcons?: boolean;

/**
* Additional params to be used in the route generation process.
*/
meta?: Record<string, string | number>;

/**
* Minimum number of breadcrumb items required for rendering.
* The component will render only if the number of items is greater than or equal to this value.
*/
minItems?: number;
};
Loading