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

[BUG] Breadcumbs Only Work on N2 Level #6497

Open
aress31 opened this issue Nov 18, 2024 · 4 comments · May be fixed by #6503
Open

[BUG] Breadcumbs Only Work on N2 Level #6497

aress31 opened this issue Nov 18, 2024 · 4 comments · May be fixed by #6503
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@aress31
Copy link

aress31 commented Nov 18, 2024

Describe the bug

It appears that the breadcrumb component doesn't display anything when the array contains only a single element. This undermines the consistency of the layout, as I would like to maintain a consistent breadcrumb style across all pages, even when there is only one breadcrumb item. Could you kindly add a property to define the minimum number of elements required for the breadcrumb to be displayed?

Steps To Reproduce

import { Toolbar } from "@mui/material";
import { useBreadcrumb } from "@refinedev/core";
import { Breadcrumb, ThemedLayoutV2 } from "@refinedev/mui";
import { Outlet } from "react-router-dom";

const Header = () => {
  const breadcumb = useBreadcrumb();
  console.log(breadcumb);

  return (
    <Toolbar>
      <Breadcrumb showHome meta={{ projectId: "123" }} key={1} />
    </Toolbar>
  );
};

const SidebarLayout = () => (
  <ThemedLayoutV2 Header={Header}>
    <Outlet context={{ component: "main", sx: { flex: 1 } }} />
  </ThemedLayoutV2>
);

export default SidebarLayout;

Expected behavior

See above.

Packages

Latest.

Additional Context

No response

@aress31 aress31 added the bug Something isn't working label Nov 18, 2024
@alicanerdurmaz
Copy link
Member

Hello, @aress31, Thanks for the detailed explanation!

Yes, It would be great! Do you want to work on this?

Source code:

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

@alicanerdurmaz alicanerdurmaz added the good first issue Good for newcomers label Nov 18, 2024
@aress31
Copy link
Author

aress31 commented Nov 18, 2024

Sure

@alicanerdurmaz
Copy link
Member

alicanerdurmaz commented Nov 20, 2024

We discussed with the core team and decided that the implementation would be better as follows:

  1. <Breadcrumb /> should take a new prop as minItems(or better name) and the breadcrumbs.length === 1 check should be compared with this new prop.

  2. new prop's type should be added to @refinedev/ui-types

/** 
* Minimum number of breadcrumb items required for rendering. Component won't render if items * are less than this number. 
*/
minItems?: number;
  1. All UI packages should be updated
  1. All tests should be updated. If possible, it should be implemented in the @refinedev/ui-tests package; otherwise, it can be implemented in the package's own test file.

  2. All documentation should be updated.

I’d also like to point out that <Breadcrumb /> can be globally configurable by passing a prop to the <Refine /> component.

You can find usage examples in the documentation here.


If I missed anything, please feel free to update it 🚀

@aress31
Copy link
Author

aress31 commented Nov 20, 2024

@alicanerdurmaz, I’ve made further updates to the PR, and it’s now ready for your review. However, I have a few points:

  1. I’m unsure how to implement the requested changes in https://github.com/refinedev/refine/tree/master/packages/core/src/hooks/breadcrumb. Any clarification or guidance would be appreciated.

  2. One (1) test is failing, and I suspect it’s because I haven’t yet addressed the issues mentioned in https://github.com/refinedev/refine/tree/master/packages/core/src/hooks/breadcrumb. Could you kindly review that part?

    > @refinedev/mui:test --testNamePattern=breadcrumb --onlyFailures
    
    @refinedev/mui: > @refinedev/[email protected] test D:\Github\refine\packages\mui
    @refinedev/mui: > jest --passWithNoTests --runInBand "--testNamePattern=breadcrumb" "--onlyFailures"
    @refinedev/mui:  FAIL   mui  src/components/breadcrumb/index.spec.tsx
    @refinedev/mui:   ● Console
    @refinedev/mui:     console.error
    @refinedev/mui:       Warning: `ReactDOMTestUtils.act` is deprecated in favor of `React.act`. Import `act` from `react` instead of `react-dom/test-utils`. See https://react.dev/warnings/react-dom-test-utils for more info.
    @refinedev/mui: 
    @refinedev/mui:       11 |   return render(
    @refinedev/mui:       12 |     <Routes>
    @refinedev/mui:     > 13 |       <Route path="/:resource/:action" element={children} />
    @refinedev/mui:          |                                        ^
    @refinedev/mui:       14 |     </Routes>,
    @refinedev/mui:       15 |     {
    @refinedev/mui:       16 |       wrapper: TestWrapper(wrapperProps),
    @refinedev/mui:
    @refinedev/mui:       at printWarning (../../node_modules/react-dom/cjs/react-dom-test-utils.development.js:71:30)
    @refinedev/mui:       at error (../../node_modules/react-dom/cjs/react-dom-test-utils.development.js:45:7)
    @refinedev/mui:       at actWithWarning (../../node_modules/react-dom/cjs/react-dom-test-utils.development.js:1736:7)
    @refinedev/mui:       at ../../node_modules/@testing-library/react/dist/act-compat.js:63:25
    @refinedev/mui:       at renderRoot (../../node_modules/@testing-library/react/dist/pure.js:159:26)
    @refinedev/mui:       at render (../../node_modules/@testing-library/react/dist/pure.js:246:10)
    @refinedev/mui:       at V (../ui-tests/src/tests/breadcrumb.tsx:13:40)
    @refinedev/mui:       at Object.<anonymous> (../ui-tests/src/tests/breadcrumb.tsx:33:42)
    @refinedev/mui:   ● Breadcrumb › [@refinedev/ui-tests] Common Tests / CRUD Create › should render successfully when at least 1 breadcrumb item is present
    @refinedev/mui:     TestingLibraryElementError: Unable to find an element with the text: Posts. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.
    @refinedev/mui:     Ignored nodes: comments, script, style
    @refinedev/mui:     <body>
    @refinedev/mui:       <div />
    @refinedev/mui:     </body>
    @refinedev/mui:
    @refinedev/mui:       50 |
    @refinedev/mui:       51 |     it("should render breadcrumb items with link", async () => {
    @refinedev/mui:     > 52 |       const { container } = renderBreadcrumb(<Breadcrumb />, {
    @refinedev/mui:          |        ^
    @refinedev/mui:       53 |         resources: [{ name: "posts", list: DummyResourcePage }],
    @refinedev/mui:       54 |         routerInitialEntries: ["/posts/create"],
    @refinedev/mui:       55 |       });
    @refinedev/mui:
    @refinedev/mui:       at Object.getElementError (../../node_modules/@testing-library/dom/dist/config.js:37:19)
    @refinedev/mui:       at ../../node_modules/@testing-library/dom/dist/query-helpers.js:76:38
    @refinedev/mui:       at ../../node_modules/@testing-library/dom/dist/query-helpers.js:52:17
    @refinedev/mui:       at ../../node_modules/@testing-library/dom/dist/query-helpers.js:95:19
    @refinedev/mui:       at Object.<anonymous> (../ui-tests/src/tests/breadcrumb.tsx:52:8)
    @refinedev/mui: Test Suites: 1 failed, 2 skipped, 1 of 3 total
    @refinedev/mui: Tests:       1 failed, 22 skipped, 8 passed, 31 total
    @refinedev/mui: Snapshots:   0 total
    @refinedev/mui: Time:        16.606 s, estimated 38 s
    @refinedev/mui: Ran all test suites with tests matching "breadcrumb".
    @refinedev/mui:  ELIFECYCLE  Test failed. See above for more details.
    
  3. I’m finding it difficult to work on the PR as I don’t have full visibility of the changes. I’ve followed the contributing guide, but running pnpm dev --scope @refinedev/mui doesn’t start a local server where I can preview the changes. Could you provide guidance on this? It would also be helpful if the guide could be improved for future contributors.

@BatuhanW BatuhanW added this to the December '24 Release milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants