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

[SelectInput] Only attach click handler to label if a labelId is passed #30239

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

johsunds
Copy link
Contributor

Spent the last couple of hours trying to figure out why my select input was getting focused when I clicked another completely unrelated element. Turns out, there was an element with id="undefined" for some reason, which caused the SelectInput to add this click listener to it since it calls .getElementById(undefined).

A rare error I suppose, but it couldn't hurt to fix it here 😄

@mui-pr-bot
Copy link

Bundle size will be reported once CircleCI build #329146 finishes.

Generated by 🚫 dangerJS against ea0f85f

@danilo-leal danilo-leal added the component: select This is the name of the generic UI component, not the React module! label Feb 1, 2022
@michaldudak
Copy link
Member

Hi @johsunds, sorry for the delayed response. Would you mind creating before/after sandboxes to help test the changes? You can use this sandbox to build a version with your changes included.

Also, a unit test to verify the implemented behavior would be needed.

@michaldudak michaldudak added the PR: needs test The pull request can't be merged label Feb 3, 2022
@johsunds
Copy link
Contributor Author

johsunds commented Feb 3, 2022

Hi @michaldudak, thanks for the response. Here are the before/after sandboxes.

I will add a unit test later, when I have time.

@mui-bot
Copy link

mui-bot commented Feb 3, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 548693e

@eps1lon
Copy link
Member

eps1lon commented Feb 3, 2022

I think you need to update the branch (e.g. by rebasing).

@johsunds johsunds force-pushed the fix-select-label-click-handler branch from b2600ee to 7b94d37 Compare February 3, 2022 23:06
@siriwatknp
Copy link
Member

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/material/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@eps1lon eps1lon added bug 🐛 Something doesn't work and removed PR: needs test The pull request can't be merged labels Feb 4, 2022
@siriwatknp siriwatknp merged commit 40885ed into mui:master Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants