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

[docs][material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumbs #42973

Merged
merged 9 commits into from
Sep 13, 2024
Merged

[docs][material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumbs #42973

merged 9 commits into from
Sep 13, 2024

Conversation

Sergio16T
Copy link
Contributor

@Sergio16T Sergio16T commented Jul 16, 2024 •
edited by ZeeshanTamboli
Loading

@mui-bot
Copy link

mui-bot commented Jul 16, 2024 •
edited
Loading

Netlify deploy preview

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 3707d94

@ZeeshanTamboli ZeeshanTamboli added package: material-ui Specific to @mui/material enhancement This is not a bug, nor a new feature new feature New feature or request component: breadcrumbs This is the name of the generic UI component, not the React module! and removed enhancement This is not a bug, nor a new feature labels Jul 17, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][breadcrumbs] add support to override onClick handler for slot elements. [material-ui][Breadcrumbs] add support to override onClick handler for slot elements. Jul 17, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Breadcrumbs] add support to override onClick handler for slot elements. [material-ui][Breadcrumbs] Add support to override onClick handler for slot elements. Jul 17, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Breadcrumbs] Add support to override onClick handler for slot elements. [material-ui][Breadcrumbs] Add support to override onClick handler of ellipsis icon Jul 17, 2024
ZeeshanTamboli
ZeeshanTamboli reviewed Jul 17, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@Sergio16T Thanks for the pull request. But before I review it, I want to ask @DiegoAndai if this feature is useful. If he agrees, I will review it, and we can merge it after his final approval. @DiegoAndai, what do you think?

@Sergio16T Sergio16T marked this pull request as draft July 19, 2024 19:12
@Sergio16T Sergio16T changed the title [material-ui][Breadcrumbs] Add support to override onClick handler of ellipsis icon [material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumb Jul 19, 2024
@Sergio16T
Copy link
Contributor Author

Sergio16T commented Jul 19, 2024 •
edited
Loading

@ZeeshanTamboli, @DiegoAndai after reviewing the ask again. I realized that the original ask from issue was to be able to create the menu functionality similar to the following Joy package example: https://mui.com/joy-ui/react-breadcrumbs/#condensed-with-menu

This functionality is currently possible without any code changes. I updated the PR to instead update the documentation to demonstrate how this can be implemented using the MUI package.

I don't believe the current ellipsis expansion behavior to show hidden breadcrumbs needs to change since the requested functionality is possible via the documentation example I added.

@Sergio16T Sergio16T marked this pull request as ready for review July 19, 2024 20:16
@Sergio16T Sergio16T changed the title [material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumb [docs][material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumb Jul 19, 2024
@Sergio16T Sergio16T changed the title [docs][material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumb [docs][material-ui][Breadcrumbs] Document CondensedWithMenu option for Material UI Breadcrumbs Jul 19, 2024
@ZeeshanTamboli ZeeshanTamboli added docs Improvements or additions to the documentation and removed new feature New feature or request labels Jul 20, 2024
ZeeshanTamboli
ZeeshanTamboli reviewed Jul 20, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great, no changes to the component code are needed 👍

DiegoAndai
DiegoAndai reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @Sergio16T, and I'm sorry for the late reply.

The approach of adding the example to the docs makes sense to me 👍🏼

May I ask you to implement a few changes:

Thanks!

ZeeshanTamboli
ZeeshanTamboli approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

It looks good to me. I pushed two commits to refine some changes and merged latest master branch. @DiegoAndai will give final review and merge.

duynguyen-kun
duynguyen-kun approved these changes Sep 13, 2024
View reviewed changes
Copy link

@duynguyen-kun duynguyen-kun left a comment

Choose a reason for hiding this comment

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

Looks LGTM, hope it can be merged soon

@duynguyen-kun duynguyen-kun mentioned this pull request Sep 13, 2024
Closed
@ZeeshanTamboli
Copy link
Member

It looks good to be merged. Let's merge it.

@ZeeshanTamboli ZeeshanTamboli merged commit e727062 into mui:master Sep 13, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

@duynguyen-kun duynguyen-kun duynguyen-kun approved these changes

@DiegoAndai DiegoAndai Awaiting requested review from DiegoAndai

Assignees
No one assigned
Labels
component: breadcrumbs This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation package: material-ui Specific to @mui/material
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[material-ui][Breadcrumbs] Allow overriding onClick button base of ellipsis icon
5 participants
@Sergio16T @mui-bot @ZeeshanTamboli @DiegoAndai @duynguyen-kun