Skip to content

Fix overlay issue #21#30

Closed
dy wants to merge 3 commits intocomponent:masterfrom
dy:master
Closed

Fix overlay issue #21#30
dy wants to merge 3 commits intocomponent:masterfrom
dy:master

Conversation

@dy
Copy link
Member

@dy dy commented Jun 1, 2015

A solution for #21.
this._overlay reference is kept untouched, supposing that once overlay is created it sticks to the dialog.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

@pirxpilot pls take a look )

@pirxpilot
Copy link
Member

Cool - let me test it quickly.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

It needs a new overlay-component

@pirxpilot
Copy link
Member

Ha - but it's way more complicated than I thought it needs to be. I was under impression we just need to remove a single line of code. Let me test a bit more.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Well I tried to find a simpler solution, but this structure seems to be better, as it clarifies things a bit.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Yes, you’re right, once is better.
Sorry about squash - I cannot get accustomed to the vim trickery.

@pirxpilot
Copy link
Member

I am getting exceptions when trying to close the dialog by clicking on overlay.

Uncaught NotFoundError: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.

These are my versions.
dialog 0.4.0
├── emitter 1.1.3
├─┬ overlay 0.3.5
│ ├── emitter 1.1.3
│ ├── domify 1.3.1
│ ├── event 0.1.4
│ └─┬ classes 1.2.1
│ └── indexof 0.0.3
├── domify 1.3.1
├── event 0.1.4
├─┬ classes 1.2.1
│ └── indexof 0.0.3
└── query 0.0.3

Do you see that too?

Also please don't add browserify dependency here. If you think that's the right thing to do submit a separate PR so that people can discuss that.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Yes, I see that too. I forgot that. It’s another one overlay’s issue component/overlay#27. It tries to remove self from the DOM where it is already detached.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Removed browserify

@pirxpilot
Copy link
Member

Nah - it's not an overlay issue.... We seem to be calling hide twice. Let me have another look.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Well yes, we call overlay.hide twice: once by clicking on the overlay itself, which removes overlay from the DOM and emits hide, which triggers dialog.hide, which finally calls dialog.hideOverlay. Ideally we should check whether overlay is hidden there to prevent second .hide call, but as far overlay doesn’t have any state API (class is not actual in that case), we have to make sure at least that calling method N times doesn’t trigger unecessary errors, like in the case of overlay.hide. Actually it is expected behaviour: if I call overlay.hide() a couple of times, I should not get errors in the console (bad example, but jquery does so with no troubles).

@dy
Copy link
Member Author

dy commented Jun 1, 2015

BTW, I fixed that error during testing overlay but forgot to add to PR.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

@pirxpilot any thoughts?

@pirxpilot
Copy link
Member

@dfcreative - Could you test #31 - I think that fixes the problem with calling overlay.hide twice when we close the dialog.

@pirxpilot
Copy link
Member

merged #31 instead

@pirxpilot pirxpilot closed this Jun 4, 2015
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