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

Update pane initialization and display logic #607

Merged
merged 2 commits into from
Oct 2, 2016

Conversation

amcgee
Copy link
Contributor

@amcgee amcgee commented Oct 1, 2016

Allow initially-hidden non-collapsible panes, sanitize togglePane 'show' parameter, and publish a subscribable event when toggling a pane. This allows for better declarative configuration without needing to utilize CSS properties or javascript callbacks.

Note : setting "open: false" still causes the sidebar to flash before disappearing. This is because the initial CSS is still set to display 'block'. To fix this, the pane initialization function would need to modify the CSS of the sidebar before it is rendered.

…ow' parameter, and publish a subscribable event when toggling a pane.
@tmcgee tmcgee self-assigned this Oct 1, 2016
@tmcgee tmcgee added this to the v2.0.0-beta.1 milestone Oct 1, 2016
@tmcgee
Copy link
Member

tmcgee commented Oct 1, 2016

@amcgee Please merge the new changes from the cmv-app develop branch and then I will merge in your enhancement. Thanks!

@amcgee
Copy link
Contributor Author

amcgee commented Oct 2, 2016

Updated, thanks!

@tmcgee tmcgee merged commit e7c3f88 into cmv:develop Oct 2, 2016
@green3g
Copy link
Member

green3g commented Oct 5, 2016

This seems to be causing a problem. When a pane is toggled via a collapse button it no longer works. It looks like line 182 will send the key, and the domEvent as a second argument.

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

The new logic doesn't check for this, and throws an error.

See my comment in the Files Changed tab

// Toggle
newDisp = (oldDisp === 'none') ? 'block' : 'none';
} else {
this.handleError({
Copy link
Member

Choose a reason for hiding this comment

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

Need to replace this error message with this:

newDisp =(domStyle.get(domNode, 'display') === 'none') ? 'block' : 'none';

Copy link
Contributor Author

@amcgee amcgee Oct 5, 2016

Choose a reason for hiding this comment

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

Good catch, thanks @roemhildtg! While accepting any type of 'show' argument and toggling if it isn't a display CSS string or boolean would work, another fix is to update the Toggle handler to call togglePane with three explicit arguments so we get togglePane(key, null, false, ). That keeps the togglePane API as strict as possible. I will open a separate pull request for discussion about that approach. EDIT: I'll comment on the PR you opened.

green3g added a commit to green3g/cmv-app that referenced this pull request Oct 5, 2016
@green3g green3g mentioned this pull request Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants