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

Added update function to options-query fixes #74. #109

Merged
merged 8 commits into from
Nov 4, 2015

Conversation

Kanaye
Copy link
Collaborator

@Kanaye Kanaye commented Oct 31, 2015

Initial problem with not updating attributes in this edgecase is fixed through updating the _state property of an VirtualElement as long as the property exsits (and changes).

Added call to observable.update() in array-oservables reset() function.
Did not find a case where this could break something, correct me if I'm wrong.

Added a function called observable._getValue() which just returns the value but does not subscribe it to the Observer. I need this for the update function of the options data-query.

Also observable.update now not subscribes observables or to be more correct it creates and drops a new Observer stack.
I couldn't find a case where it breaks something as the call that causes the subsciption is caused by and on already subscribed elements.

Fixed a bug with the new Expression.NodeWise rendering that caused some wrong updates in the select.

Added the update-function to the options-data-query and tests for it which causes following behaviour:

On update(reset, remove, ...) of the collection:

  • if the selected value is in the collection this value is used (no action will be performed).
  • if the selected value is not in the collection but there is an option with data-role="header" (=caption in the queries options) this option is used.
  • if the selected value is not in the collection and there is no caption/data-role="header"-option the value is set to the first item of the collection.

Hope I've been clear enough. Ask questions if you have some 😄.

Fixes #74.

@@ -342,6 +350,7 @@ define([
reset: function (array) {
if (arguments.length === 0) {
this.removeAll();
this.update();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should be removed because the last line in removeAll method is update() and this call will only add overhead to the performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally right. Iremoved it.

@astoilkov
Copy link
Owner

I can suggest a thing when making changes that affect core logic. I usually take the jsblocks-shopping-example replace it with the newest version of the code and run npm start and then npm run node to ensure client-side and server-side rendering and functionality is ok. I test all functionalities and refresh every page to see the server-side rendering doesn't throws any errors and everything is working fine after that.

Can you do this with these changes so we are sure nothing broke before merging this?

Joscha Rohmann added 8 commits November 3, 2015 21:15
Caused by not updating ``VirtualELement._state`` in
``_executeAttributeExpressions`` if the Element is not
Virtual. Fixed by updateing as long as '_state' exists.
Added call to observable.update() in array-observable.reset().
Require as the data-query.update() function will get called there.
An way for (internal) Methods to get the value with out beeing registered
by the Observer. For cases where you need the observable value but don't
want to get updated if it changes (In my case the update function of the options
data-query)
observable.update now will not subscribe it's observable to the current
element (e.g. data-query).
Update will check for selected value inside the updated collection.
And update the value to the first 'data-rel="header"' or when provided
'caption' option or otherwise to the first value of the collection.
Closes astoilkov#74.
astoilkov added a commit that referenced this pull request Nov 4, 2015
Added update function to options-query fixes #74.
@astoilkov astoilkov merged commit 107b09c into astoilkov:master Nov 4, 2015
@Kanaye Kanaye deleted the bug-#74 branch November 6, 2015 15:30
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.

options query do not replace option value in some situations
2 participants