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

[Chip] Support pressing delete to delete a chip #14978

Merged
merged 4 commits into from
Mar 23, 2019

Conversation

keeslinp
Copy link
Contributor

If it is a built-in feature that the chip handles the backspace key to trigger a delete event, it should probably also support the delete key. I need to handle deleting chips with the delete button, I could do it exterior to the component but I figure it would be more idiomatic to follow the existing flow.

@keeslinp keeslinp changed the title [Chip] Support pressing delete to delete a chip WIP: [Chip] Support pressing delete to delete a chip Mar 20, 2019
@keeslinp keeslinp changed the title WIP: [Chip] Support pressing delete to delete a chip [Chip] Support pressing delete to delete a chip Mar 20, 2019
@keeslinp
Copy link
Contributor Author

I'm not sure why that last test failed, looking at the log it appears that it passed and then failed to upload? I'm not familiar with how all the CI pieces work.

@joshwooding
Copy link
Member

Could you branch this off next please, we are keeping master for important bug fixes :)

@keeslinp keeslinp changed the base branch from master to next March 20, 2019 22:40
@keeslinp
Copy link
Contributor Author

Changed it. It's unfortunate that we would have to update to an unstable alpha release to get such a simple improvement.

@mui-pr-bot
Copy link

mui-pr-bot commented Mar 20, 2019

Details of bundle changes.

Comparing: f9896bc...998a822

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.01% 🔺 352,709 352,746 90,549 90,554
@material-ui/core/Paper 0.00% 0.00% 68,626 68,626 19,973 19,973
@material-ui/core/Paper.esm 0.00% 0.00% 62,358 62,358 19,073 19,073
@material-ui/core/Popper 0.00% 0.00% 30,460 30,460 10,528 10,528
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,384 17,384 5,726 5,726
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,038 1,038
@material-ui/lab 0.00% 0.00% 152,181 152,181 44,528 44,528
@material-ui/styles 0.00% 0.00% 53,837 53,837 15,615 15,615
@material-ui/system 0.00% 0.00% 17,138 17,138 4,522 4,522
Button 0.00% 0.00% 90,918 90,918 26,969 26,969
Modal 0.00% 0.00% 84,712 84,712 25,262 25,262
colorManipulator 0.00% 0.00% 3,232 3,232 1,300 1,300
docs.landing 0.00% 0.00% 51,843 51,843 11,349 11,349
docs.main 0.00% 0.00% 647,785 647,785 201,132 201,132
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 +0.01% 🔺 301,799 301,836 83,742 83,748

Generated by 🚫 dangerJS against 998a822

@oliviertassinari oliviertassinari added new feature New feature or request component: chip This is the name of the generic UI component, not the React module! labels Mar 21, 2019
@oliviertassinari
Copy link
Member

Changed it. It's unfortunate that we would have to update to an unstable alpha release to get such a simple improvement.

@keeslinp There are two reasons behind it: n°1, we create incentives for people to upgrade from v3 to v4. n°2, it's allowing the team to improve the library at a faster pace.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 21, 2019
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 22, 2019
@keeslinp
Copy link
Contributor Author

Thanks for updating that. I think a lot of the differences were from building off next then moving to master then back to next.

@oliviertassinari
Copy link
Member

@keeslinp Yes, the core components are under an important refactorization 🚧. We are moving at 100% to the usage of hooks (no classes). We are moving at 100% to mount tests (no shallow).

@oliviertassinari
Copy link
Member

@keeslinp It's a great first pull request on Material-UI 👌🏻. Thank you for working on it!

@oliviertassinari oliviertassinari merged commit 55b806b into mui:next Mar 23, 2019
@CrocoDillon
Copy link

CrocoDillon commented Aug 29, 2019

What happened to this?

Edit: never mind I messed up, it works... I don't know why though, can't seem to find this change back in Chip.js 🚶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: chip This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants