Skip to content

[ListItem] Fix primaryTogglesNestedList behavior when left checkbox is present#4947

Closed
nehalbhanushali wants to merge 1 commit intomui:masterfrom
nehalbhanushali:ListWork
Closed

[ListItem] Fix primaryTogglesNestedList behavior when left checkbox is present#4947
nehalbhanushali wants to merge 1 commit intomui:masterfrom
nehalbhanushali:ListWork

Conversation

@nehalbhanushali
Copy link
Copy Markdown

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

When set to true, primaryTogglesNestedList property toggles nested list even when checkbox is present.

The fix addresses #4753

@nehalbhanushali nehalbhanushali changed the title [ListItem] primary text toggles nested list over left checkbox check [ListItem] Fix primaryTogglesNestedList behavior when left checkbox is present Aug 10, 2016
@nehalbhanushali
Copy link
Copy Markdown
Author

nehalbhanushali commented Aug 12, 2016

@oliviertassinari can you tell me what you think about this? I mean is it worth for the master or will the next branch take care of it?

@oliviertassinari oliviertassinari added type: bug It doesn't behave as expected. and removed PR: Needs Review labels Aug 14, 2016
@oliviertassinari
Copy link
Copy Markdown
Member

@nehalbhanushali Could you have a look at #4753 (comment)? I don't think that we are going it the right direction here.

@nehalbhanushali
Copy link
Copy Markdown
Author

Thanks @oliviertassinari and @rafaelsales . I see what I missed. However, I just wanted to confirm if the checkbox should also be able to toggle the nested list (which I guess, happens with the #4753 fix ). In my opinion it should not.

@rafaelsales
Copy link
Copy Markdown

I think it's fine that the label toggles the checkbox, because solo checkboxes are toggled when you click on the label. I think the main point of that issue was that primaryTogglesNestedList was just being ignored on the presence of leftCheckbox

@nehalbhanushali
Copy link
Copy Markdown
Author

nehalbhanushali commented Aug 15, 2016

@rafaelsales Im not talking about the toggle of the checkbox by the label. I am talking about the toggle of the nested list by clicking the checkbox and not the primary text. Sorry for not being clear.

@rafaelsales
Copy link
Copy Markdown

@nehalbhanushali very very true. Seems like I exchanged a bug by another.

@oliviertassinari
Copy link
Copy Markdown
Member

@nehalbhanushali Good point.
@rafaelsales Do you want to submit another PR?

@rafaelsales
Copy link
Copy Markdown

Yes, I'll be working on it late today!

@nehalbhanushali
Copy link
Copy Markdown
Author

nehalbhanushali commented Aug 22, 2016

would this be a right approach? @oliviertassinari @rafaelsales

handleNestedListToggle = (event) => {
      event.stopPropagation();           
      event.stopPropagation();

    if (event.target.type === 'checkbox') {     
       this.setState({open: this.state.open}, () => {
       this.props.onNestedListToggle(this);
      });
      }
     else
      this.setState({open: !this.state.open}, () => {
      this.props.onNestedListToggle(this);
       });
    };         
 };

@oliviertassinari
Copy link
Copy Markdown
Member

@nehalbhanushali I think that the best fix is to revert #4988 and to follow #4753 (comment).

@nehalbhanushali
Copy link
Copy Markdown
Author

nehalbhanushali commented Aug 22, 2016

@oliviertassinari alright 👍 Although I think the revert would not be required. #4988 resolves the createLableElement part. So it needs to be there. There is just more that is required, related to the handleNestedToggle for checkbox click case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: bug It doesn't behave as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants