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

[Chip] Add skipFocusWhenDisabled prop to not allow focussing deletable chip if disabled #35065

Merged
merged 15 commits into from Nov 25, 2022
Merged

[Chip] Add skipFocusWhenDisabled prop to not allow focussing deletable chip if disabled #35065

merged 15 commits into from Nov 25, 2022

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Nov 9, 2022 •
edited

Fixes: #35038

@mui-bot
Copy link

mui-bot commented Nov 9, 2022 •
edited

Messages
📖 Netlify deploy preview: https://deploy-preview-35065--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against b5086bc

@zannager zannager added the component: chip This is the name of the generic UI component, not the React module! label Nov 9, 2022
ZeeshanTamboli
ZeeshanTamboli previously requested changes Nov 9, 2022
View reviewed changes
packages/mui-material/src/Chip/Chip.js Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli added the bug 🐛 Something doesn't work label Nov 9, 2022
ZeeshanTamboli
ZeeshanTamboli reviewed Nov 9, 2022
View reviewed changes
packages/mui-material/src/Chip/Chip.js Outdated Show resolved Hide resolved
packages/mui-material/src/Chip/Chip.js Outdated Show resolved Hide resolved
@ZeeshanTamboli ZeeshanTamboli dismissed their stale review November 9, 2022 15:34

changes applied

@ZeeshanTamboli ZeeshanTamboli changed the title [Chip] avoided focus if chip is disabled [Chip] Don't focus if disabled Nov 9, 2022
ZeeshanTamboli
ZeeshanTamboli approved these changes Nov 12, 2022
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.

I pushed a change and improved the test. @sai6855 Some other MUI team member may review it again. From my side, it looks good. Great first contribution!

@ZeeshanTamboli ZeeshanTamboli changed the title [Chip] Don't focus if disabled [Chip] Don't focus deletable chip if disabled Nov 12, 2022
@sai6855
Copy link
Contributor Author

sai6855 commented Nov 12, 2022

Hey thanks for approval, looking forward for more contribution

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 21, 2022

Hi @ZeeshanTamboli I saw a new pr for 5.10.15 version release, will this PR be a part of 5.10.15 release?

@ZeeshanTamboli
Copy link
Member

Hi @ZeeshanTamboli I saw a new pr for 5.10.15 version release, will this PR be a part of 5.10.15 release?

It's part of the release only if it is merged. I wouldn't want to merge PRs until there is an approval from a MUI Core team member that it looks good. I am just a MUI maintainer (not part of the core team). I just try to review PRs to polish it up. But the final go ahead is from a core member.

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 21, 2022

Ah got it, no issues

mnajdova
mnajdova reviewed Nov 22, 2022
View reviewed changes
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

In order to avoid breaking changes in the behavior compared to before, how about we add a prop that will determine whether the disabled chip should be focusable (and have it true by default). We have something similar in the @mui/base's Button for example: focusableWhenDisabled. cc @michaldudak for additional thoughts.

@michaldudak
Copy link
Member

michaldudak commented Nov 23, 2022 •
edited

I agree, this should be configurable. Ideally, we would use the same prop as in ButtonUnstyled, focusableWhenDisabled, but making it true by default goes against our style guide. I propose skipFocusWhenDisabled instead, which we can make false by default.
We can unify the props in the next major then.

@sai6855
Copy link
Contributor Author

sai6855 commented Nov 23, 2022

@michaldudak as you suggested i added skipFocusWhenDisabled prop with default value as false.

@ZeeshanTamboli ZeeshanTamboli changed the title [Chip] Don't focus deletable chip if disabled [Chip] Add skipFocusWhenDisabled prop to allow not to focus deletable chip if disabled Nov 25, 2022
@ZeeshanTamboli ZeeshanTamboli added enhancement This is not a bug, nor a new feature and removed bug 🐛 Something doesn't work labels Nov 25, 2022
@ZeeshanTamboli ZeeshanTamboli changed the title [Chip] Add skipFocusWhenDisabled prop to allow not to focus deletable chip if disabled [Chip] Add skipFocusWhenDisabled prop to not allow focussing deletable chip if disabled Nov 25, 2022
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Nov 25, 2022
Closed
2 tasks
@ZeeshanTamboli ZeeshanTamboli merged commit 2861715 into mui:master Nov 25, 2022
daniel-rabe pushed a commit to daniel-rabe/material-ui that referenced this pull request Nov 29, 2022
@sai6855 @ZeeshanTamboli
[Chip] Add skipFocusWhenDisabled prop to not allow focussing deleta…
fb50bec 
…ble chip if disabled (mui#35065)

Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
feliperli pushed a commit to jesrodri/material-ui that referenced this pull request Dec 6, 2022
@sai6855 @ZeeshanTamboli
[Chip] Add skipFocusWhenDisabled prop to not allow focussing deleta…
1eb6e52 
…ble chip if disabled (mui#35065)

Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>
Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Dec 20, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaldudak michaldudak michaldudak left review comments

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

@mnajdova mnajdova Awaiting requested review from mnajdova

Assignees
No one assigned
Labels
component: chip This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Chip] When disabled it still receives focus
6 participants
@sai6855 @mui-bot @ZeeshanTamboli @michaldudak @mnajdova @zannager