Skip to content

Conversation

megheaiulian
Copy link
Collaborator

@megheaiulian megheaiulian commented Nov 2, 2017

Limit resize notifications to selected tab and check for visible before firing a resize.

fixes #30

@nomego nomego requested a review from plequang November 2, 2017 14:39
Copy link
Contributor

@plequang plequang left a comment

Choose a reason for hiding this comment

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

Seems to be a typo with isVisible and _isVisible

},

_onDescendantIronResize(event) {
if (this._notifyingDescendant || !this.isVisible) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be this._isVisible ?

}
});

if (!this.isVisible) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@megheaiulian
Copy link
Collaborator Author

@plequang Fixed typos.

Prevent firing resize upwards from a descendent when it is not a tab or cosmoz-tabs is not visible
Resize
@megheaiulian megheaiulian force-pushed the feature/NEO-39-resizable branch from 8d29935 to a6a4de3 Compare November 3, 2017 08:44
Copy link
Contributor

@plequang plequang left a comment

Choose a reason for hiding this comment

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

See comments

return Boolean(this.offsetWidth || this.offsetHeight);
},

_onDescendantIronResize(event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Redefining _onDescendantIronResize in tabbable behavior is a bit confusing.
Because, looking at cosmoz-tab behaviors:

behaviors: [
	Cosmoz.TabbableBehavior,
	Cosmoz.TabbedBehavior
],

one might think this overridden method will also apply to cosmoz-tab, which is not the case (because the last behavior takes precedence).

So because tabbable behavior is shared between cosmoz-tabs and cosmoz-tab, it would be much clearer if _onDescendantIronResize was redefined in cosmoz-tabs.

}
});

if (!this._isVisible) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be also possible to not notify descendants when this._isVisible is false ?

return;
}

if (!Polymer.Settings.useShadow) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If redefining _onDescendantIronResize, maybe you should consider this PR PolymerElements/iron-resizable-behavior#35

@plequang plequang merged commit 5c2e9d6 into Neovici:master Nov 3, 2017
@megheaiulian megheaiulian deleted the feature/NEO-39-resizable branch November 3, 2017 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resize notifications should be limited

3 participants