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

[ListItem] Fix triggers onTouchTap when disabled #7486

Merged
merged 6 commits into from
Jul 21, 2017
Merged

[ListItem] Fix triggers onTouchTap when disabled #7486

merged 6 commits into from
Jul 21, 2017

Conversation

jonashartwig
Copy link

@jonashartwig jonashartwig commented Jul 20, 2017

I think it was forgotten to send disabled to EnhancedButton as it has internal logic to not trigger onTouchTap if disabled is set to true.

Closes #7449.

@jonashartwig jonashartwig reopened this Jul 20, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The fix could be a one line change:

  render() {
    const {
      autoGenerateNestedIndicator,
      children,
      containerElement,
      disabled,
      disableKeyboardFocus,
      hoverColor, // eslint-disable-line no-unused-vars
      initiallyOpen, // eslint-disable-line no-unused-vars
      innerDivStyle,
      insetChildren, // eslint-disable-line no-unused-vars
      leftAvatar,
      leftCheckbox,
      leftIcon,
      nestedItems,
      nestedLevel,
      nestedListStyle,
      onKeyboardFocus, // eslint-disable-line no-unused-vars
      isKeyboardFocused, // eslint-disable-line no-unused-vars
      onMouseEnter, // eslint-disable-line no-unused-vars
      onMouseLeave, // eslint-disable-line no-unused-vars
      onNestedListToggle, // eslint-disable-line no-unused-vars
      onTouchStart, // eslint-disable-line no-unused-vars
+     onTouchTap, // eslint-disable-line no-unused-vars
      rightAvatar,

@oliviertassinari oliviertassinari changed the title #7449 - listitem triggers ontouchtap if disabled [ListItem] Fix triggers onTouchTap when disabled Jul 20, 2017
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: list This is the name of the generic UI component, not the React module! PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jul 20, 2017
@mui mui deleted a comment from jonashartwig Jul 20, 2017
@mui mui deleted a comment from jonashartwig Jul 20, 2017
@mui mui deleted a comment from jonashartwig Jul 20, 2017
@jonashartwig
Copy link
Author

you are right. Do you want me to open a new PR with only one commit?

regards

@oliviertassinari
Copy link
Member

As you prefer, I will squash the commits if needed.

@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jul 21, 2017
@oliviertassinari oliviertassinari merged commit c759b7d into mui:master Jul 21, 2017
@oliviertassinari
Copy link
Member

@jonashartwig Thanks

@hwo411
Copy link

hwo411 commented Jul 21, 2017

@oliviertassinari EnchancedButton part is unreachable when disabled is true https://github.com/callemall/material-ui/pull/7486/files#diff-a3d0f303a92012b705e6df2b002e8a0eR722
This PR contains mistake.

@oliviertassinari
Copy link
Member

@hwo411 True, I just thought it's inoffensive to have that logic.

@jonashartwig
Copy link
Author

yes, but then it does not change anything, it will send all values that are falsey as disabled then. Is it supposed to be fixed? I dont think it will break anything

djbuckley added a commit to manchesergit/material-ui that referenced this pull request Jul 24, 2017
* call_em_all_-_master/master:
  fix: comment typo (mui#7523)
  [Docs] Add v0.18.7 to versions.json
  v0.18.7
  [CHANGELOG] Prepare v0.18.7
  [ListItem] Fix triggers onTouchTap when disabled (mui#7486)
  [Popover] Scroll Container issue (mui#7472)
  [Table] Don't set height to tbody (mui#7484)
  docs: fix typo in Avatar readme (mui#7478)

# Conflicts:
#	src/Popover/Popover.js
# merged both versions
adam187 added a commit to adam187/material-ui that referenced this pull request Nov 21, 2017
…om-build

* fix/broken-touch-scroll-on-nested-menu-items:
  [Menu] Ignore loosing focus on click away for item with menu items
  [examples] Remove browserify
  Fixes mui#7559 (mui#7560)
  [docs] Update ROADMAP (mui#7543)
  [Table] Row click on empty cell to not die in IE (mui#7520)
  Internal(EnhancedSwitch): fix checked prop (mui#7499)
  fix: comment typo (mui#7523)
  [Docs] Add v0.18.7 to versions.json
  v0.18.7
  [CHANGELOG] Prepare v0.18.7
  [ListItem] Fix triggers onTouchTap when disabled (mui#7486)
  [Popover] Scroll Container issue (mui#7472)
  [Table] Don't set height to tbody (mui#7484)
  docs: fix typo in Avatar readme (mui#7478)
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: list 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.

None yet

3 participants