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

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

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

Conversation

panarch
Copy link

@panarch panarch commented Aug 16, 2015

DOMElement.onShow should not update display property to block when its node.isShown() is false

DOMElement.onShow should not update display property to block when its node.isShown() is false
@@ -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.

@panarch
Copy link
Author

panarch commented Aug 18, 2015

@alexanderGugel

If a parent node is hidden, all its children are hidden (this._shown === false)

Currently, although Dispatch#show calls Node#onShow and DOMElement#onShow, it does not affect Node#_shown because Dispatch does not call Node#show. And it's same in case of Dispatch#hide
So, the assumption was already broken and I just used that state.
I intended affecting to current structure as less as I can, and the result is current PR.

But in this time, if you want to fix not only for the issue I suggested, but also change the current structure to fit the assumption, I have an another idea.

I think it is not possible to fit the assumption in current structure because actual Node#_shown state is more than two;

  1. Node is shown
  2. Parent is shown, Node is hidden
  3. Node is hidden
    or
  4. Node is shown
  5. Parent is hidden, Node is shown
  6. Node is hidden

It seems... first one might be more intuitive... hmm.. may be not, anyway.

The main key point would be this:
Parent node's visible state change should not affect child node's visible(hidden) state which is manually made.
And once Node#hide called by user, it should be preserved when user calls ParentNode#show.

I think my PR can be thought as a hotfix to simply fix this without touching the current structure.
How about applying this PR first and then thinking about the solution to do refactoring of Node#_shown state to be more intuitive?

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.

2 participants