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

Fix broken pane toggle #613

Merged
merged 3 commits into from Oct 6, 2016

Conversation

Projects
None yet
4 participants
@roemhildtg
Copy link
Member

roemhildtg commented Oct 5, 2016

Panes are not toggleable by the button since #607

@amcgee
Copy link
Contributor

amcgee left a comment

Good catch! This approach (accepting arbitrary input and toggling if it isn't something we recognize) should conflate the last two else if/else conditions since they are logically identical.

Alternatively, to keep the strict togglePane API which only accepts reasonable 'show' arguments I would suggest instead changing line 182 from on(this.collapseButtons[key], 'click', lang.hitch(this, 'togglePane', key)); to on(this.collapseButtons[key], 'click', lang.hitch(this, 'togglePane', key, null, false)); and line 231 from } else if (show === undefined) { to } else if (show === undefined || show === null) {.

Either technically works, I leave the choice of which approach is preferred up to the maintainers.

error: 'Invalid type passed as "show" property of "togglePane" function : ' + typeof(show)
});
return;
newDisp = (domStyle.get(domNode, 'display') === 'none') ? 'block' : 'none';

This comment has been minimized.

@amcgee

amcgee Oct 5, 2016

Contributor

This is logically identical to the above case, so if we want to accept arbitrary input for the 'show' parameter we should just remove this error condition and change the else if (show === undefined) case to else

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 5, 2016

Agreed, I like the second approach:

on(this.collapseButtons[key], 'click', lang.hitch(this, 'togglePane', key, null, false));
@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 5, 2016

I like the second approach as well.

For consistency, the suppressEvents parameter should also be passed to togglePane method in the response to the topic viewer/togglePane.

@amcgee

This comment has been minimized.

Copy link
Contributor

amcgee commented Oct 5, 2016

Agreed!

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 5, 2016

Just noticed, you guys both have the same last names...are you related? 😃

@roemhildtg roemhildtg force-pushed the roemhildtg:fix-broken-pane-toggle branch 2 times, most recently from f6e7844 to 942117b Oct 5, 2016

@roemhildtg roemhildtg force-pushed the roemhildtg:fix-broken-pane-toggle branch from 942117b to 1fafd15 Oct 5, 2016

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 5, 2016

@roemhildtg can you also add the additional argument here.

@DavidSpriggs

This comment has been minimized.

Copy link
Member

DavidSpriggs commented Oct 6, 2016

@amcgee merge base branch. thanks guys!

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 6, 2016

^^^ @roemhildtg that comment is for you. Hoping you can also include the request in my comment.

@DavidSpriggs

This comment has been minimized.

Copy link
Member

DavidSpriggs commented Oct 6, 2016

yep! sorry @roemhildtg my bad, thanks!

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 6, 2016

Sorry! So just to be sure, that should be changed to this.togglePane(args.pane, args.show, false); correct? Since we dont want to suppress the event?

@tmcgee

This comment has been minimized.

Copy link
Member

tmcgee commented Oct 6, 2016

@roemhildtg how about passing args.suppressEvents?

@roemhildtg

This comment has been minimized.

Copy link
Member

roemhildtg commented Oct 6, 2016

Okay, gotcha.

@tmcgee tmcgee merged commit 3d12b9a into cmv:develop Oct 6, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tmcgee tmcgee added the bug label Oct 6, 2016

@tmcgee tmcgee added this to the v2.0.0-beta.1 milestone Oct 6, 2016

@roemhildtg roemhildtg deleted the roemhildtg:fix-broken-pane-toggle branch Oct 7, 2016

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