-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix(mui): Fixed condition preventing Breadcrumb text display #6503
base: master
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: f952ed3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @aress31 thanks for taking time to create issues and the PR.
In your PR, please do not include any irrelevant changes. Only the changes related to your fix and specs.
Imports like import HomeOutlined from "@mui/icons-material/HomeOutlined";
are not coincidence. They are intentionally like that because of bundling issues with mui library.
@aress31 For tests, you can update the tests in |
@BatuhanW besides reverting the imports, what else do you want me to change? I think refactoring the file as I did help with modularity and readability - killing two birds with one stone. |
As requested, I focused solely on the fix. However, I recommend reviewing my previous commits with the refactoring. Some styles, such as default padding and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (breadcrumbs.length === 1) { | ||
return null; | ||
} | ||
if (breadcrumbs.length < minItems) return null; |
There was a problem hiding this comment.
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.
if (breadcrumbs.length < minItems) return null; | |
if (breadcrumbs.length <= minItems) return null; |
- default should be set to 2
minItems = 2
I believe second option is better because it fits minItems
name.
There was a problem hiding this comment.
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. :)
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
What is the new behavior?
fixes #6497
Notes for reviewers
The following tasks have not been performed, kindly complete them to provide me with a how-to example: