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

Observe attributes and update options on change. Update default text on every attribute update. #128

Closed
wants to merge 3 commits into from

Conversation

vstene
Copy link

@vstene vstene commented Jul 30, 2015

No description provided.

@vstene vstene changed the title Observe options Observe attributes and update options on change. Update default text on every attribute update. Jul 30, 2015
@leocaseiro
Copy link
Owner

Hi @vstene. Thanks for your PR.

I've just take this project and I would like to know about your scenario. Could you please explain me why would be necessary? Would you mind creating a plunkr for me?

You can fork from this example though: http://plnkr.co/edit/1U8KCH?p=preview

Thanks

@leocaseiro leocaseiro self-assigned this Feb 27, 2016
@vstene
Copy link
Author

vstene commented Mar 1, 2016

Hello @leocaseiro,

I have a multi language app and I need to refresh the text attributes on language change. The problem seems to be that the attributes are not binded.

For example, when changing the scope variable the text is not updated:
http://plnkr.co/edit/xRAl6ho62LJdScMZ3Ils?p=preview

From the current version of my fork the binding is applied via attr.$observe:
http://plnkr.co/edit/yOBllVXEEVhklzw7Xb5V?p=preview

Hope this helps.

@leocaseiro
Copy link
Owner

Hi @vstene thank you for your reply.

I liked it. I'm going to resolve some conflicts and them I'll accept your PR.

I just would remove the reference from chosen.set_default_text() which gives me a warning:

TypeError: Cannot read property 'set_default_text' of null

@leocaseiro
Copy link
Owner

So we don't need the methods disableWithMessage() and removeEmptyMessage() as the updateMessage() would work for both, right?

@leocaseiro
Copy link
Owner

I created the branch 128-vind-placeholder-text-single which should fix your issue. I resolved all conflicts with the last PR's. Could you please double check if works for you?
So I'll merge into master and release 1.2.1 with this enhancement

Thanks

@leocaseiro leocaseiro added this to the 1.2.1 milestone Mar 2, 2016
@leocaseiro leocaseiro removed the 1.2.1 label Mar 2, 2016
@vstene
Copy link
Author

vstene commented Mar 10, 2016

I don't remember the reason for the set_default_text(), but it seems to work now without.

leocaseiro added a commit that referenced this pull request Mar 12, 2016
#128 Fix Observe attributes and update options on change. Update default text on every attribute update thanks @vstene
@leocaseiro leocaseiro reopened this Mar 17, 2016
@leocaseiro leocaseiro closed this Mar 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants