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

.disabled is improperly set when reusing a settings object #25

Open
dovrosenberg opened this issue Feb 20, 2015 · 6 comments
Open

.disabled is improperly set when reusing a settings object #25

dovrosenberg opened this issue Feb 20, 2015 · 6 comments

Comments

@dovrosenberg
Copy link
Contributor

When I reuse a settings object by passing it into initDialog() and then after that dialog closes using it again to initialize a new one, the buttons all become disabled.

Perhaps this is a bad practice, as the settings object is modified by initDialog(), but it seems like it should be doable in order to limit the need to recreate the same object over and over.

In any case, the problem seems to be this line in initDialog() doesn't work if .disabled is already a ReactiveVar:

newButton.disabled = new ReactiveVar((info.buttons[button].disabled === undefined || info.buttons[button].disabled === false) ? false : true);

Perhaps it should be something like:
newButton.disabled = new ReactiveVar((info.buttons[button].disabled === undefined || info.buttons[button].disabled === false || (info.buttons[button].disabled instanceof ReactiveVar && info.buttons[button].disabled.curValue===false)) ? false : true);

@jchristman
Copy link

+1 please merge the solution. I am having this same problem.

@pahans
Copy link
Owner

pahans commented Feb 23, 2015

i am busy with a personal matter for few days. can some one test this PR and confirm it?, so i can merge it. thanks in advance.

@jchristman
Copy link

Error with code. See my comments on the PR.

pahans added a commit that referenced this issue Feb 23, 2015
@jchristman
Copy link

@pahans, do you know when you might get a chance to publish to atmosphere? Also, I think you can close this issue now.

@jchristman
Copy link

@dovrosenberg, seeing as the atmosphere package was never updated with your change and another PR I made was never merged, I have created a new atmosphere package with yours and my change in it. See https://atmospherejs.com/jchristman/reactive-modal.

@dovrosenberg
Copy link
Contributor Author

Thanks, @jchristman

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

No branches or pull requests

3 participants