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.

Sign up for GitHub

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

Jump to bottom

fix: Fix DOMElement#onShow, Node#show, hide (#470) #471

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix: Fix DOMElement#onShow, Node#show, hide (#470) #471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/Node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,8 +922,8 @@ Node.prototype._vecOptionalSet = function _vecOptionalSet (vec, index, val) {
* @return {Node} this
*/
Node.prototype.show = function show () {
Dispatch.show(this.getLocation());
this._shown = true;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this._shown shouldn't be set here at all. Instead node._setShown(true); should be called in Dispatch#show.

Dispatch.show(this.getLocation());
return this;
};

Expand All @@ -937,8 +937,8 @@ Node.prototype.show = function show () {
* @return {Node} this
*/
Node.prototype.hide = function hide () {
Dispatch.hide(this.getLocation());
this._shown = false;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Dispatch.hide(this.getLocation());
return this;
};

Expand Down
2 changes: 1 addition & 1 deletion dom-renderables/DOMElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ DOMElement.prototype.onDismount = function onDismount() {
* @return {undefined} undefined
*/
DOMElement.prototype.onShow = function onShow() {
this.setProperty('display', 'block');
if (this._node.isShown()) this.setProperty('display', 'block');
Copy link
Member

Choose a reason for hiding this comment

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

This is an unnecessary change IMO. When DOMElement#onShow is being called, we can assume that the Node is being shown. No need to check again.

Copy link
Author

Choose a reason for hiding this comment

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

@alexanderGugel Thanks for fast responding!

Currently, once a parent node#show called, the parent node calls all the children's node#onShow and child nodes` DOMElement#onShow although some child nodes are manually hidden which should not be affected by a parent.

So, it can break child nodes' visible state, there's an example to see it
#470

1. Modify DOMElement.onShow not to change property when its Node.isShown() is false
2. Modify Dispatch.show to ignore nodes which Node.isShown() is false

In that link, I chose the first way, but if I take the second to fix, it might be like this below.

Rather than changing DOMElement#onShow, changing Dispatch#show

Wrapping by if (node.isShown()) statement

if (node.isShown()) {
    if (node.onShow) node.onShow();

    var components = node.getComponents();
    for (var i = 0, len = components.length ; i < len ; i++)
        if (components[i] && components[i].onShow)
            components[i].onShow();
}

Both cases, updating Node#_shown value should be processed before than calling Dispatch#show

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Currently, once a parent node#show called, the parent node calls all the children's node#onShow and child nodes` DOMElement#onShow although some child nodes are manually hidden which should not be affected by a parent.

Good point. I doubt there is a good solution to this in the current architecture. If a parent node is hidden, all its children are hidden (this._shown === false). I think that's an assumption we have to make and I don't think we want to change that.

What I was suggesting (seems like I misunderstood the intention behind this PR), is that the node's show state should be updated in the Dispatch, not in the Node's show method.

};

/**
Expand Down