Skip to content

Append element on show instead of on init.#26

Merged
pirxpilot merged 2 commits intocomponent:masterfrom
dy:master
Jun 1, 2015
Merged

Append element on show instead of on init.#26
pirxpilot merged 2 commits intocomponent:masterfrom
dy:master

Conversation

@dy
Copy link
Member

@dy dy commented Jun 1, 2015

Related to component/dialog#21

@dy
Copy link
Member Author

dy commented Jun 1, 2015

@pirxpilot, @juliangruber, @rauchg please merge)
If you grant access to this repo, I won’t bother you anymore, sorry )

@pirxpilot
Copy link
Member

Doesn't it make things even worse? Won't we be adding overlay multiple times if someone calls show multiple times? I think it would better to fix hide in the way I described in component/dialog#21

@dy
Copy link
Member Author

dy commented Jun 1, 2015

An element can’t be added multiple times to the DOM. Just test in console:

var d = document.createElement('div');
document.body.appendChild(d);
document.body.appendChild(d);

That is safe behavior.

For now if overlay was hidden (via .hide()), it is no more able to be shown once again. It is inconsistent behavior and factually a hidden state.

I have a solution for dialog, but it needs the new version of the overlay-component first.

@pirxpilot
Copy link
Member

Good point.

Still why not fix Dialog.hideOverlay - so that it does not reset _overlay property to null (which happens to be the root cause of component/dialog#21)

It seems less disruptive than changing this component.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

I’ve made a PR to component-dialog component/dialog#30.
It seems to be the solution similar to the one you suggest.
There were some troubles of recursive call on closing a dialog.

@pirxpilot
Copy link
Member

You know what: you're right on this one. It only makes sense that we add element in show if we are removing it in hide - I was under impression we are just adding/removing the 'hidden' class.
I'll take it into master and release it, but please clean up the white spaces and push an update.

Also could you hold on for a moment on dialog changes. I'd like to have another look.

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Right, whitespaces. Done.

@pirxpilot pirxpilot merged commit bd2c2fc into component:master Jun 1, 2015
@pirxpilot
Copy link
Member

Thanks. 0.3.5 tagged.
Next time feel free to unleash your git squash superpowers ;-)

@dy
Copy link
Member Author

dy commented Jun 1, 2015

Got it, thanks)

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