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

Add error callback for raw data #94

Merged
merged 23 commits into from Jun 24, 2013

Conversation

dianatatu
Copy link
Contributor

Need

Besides success callback, we need to have an error callback for errors in raw data.
This is currently needed in #7849 .

Solution

  • add erorr callback that triggers an error event.

Files changed

  • thehole/app/core/raw_data.coffee

cc @aismail

@dianatatu
Copy link
Contributor Author

@aismail please review :)

@@ -149,12 +149,17 @@ define [], () ->
# Trigger a no_data event when the response is empty
if _.isEmpty(data)
@trigger('no_data', @)

error_callback = (xhr, response_status, error_string) =>
@trigger('error', @, @, xhr.status)
Copy link
Contributor

Choose a reason for hiding this comment

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

This https://github.com/uberVU/mozaic/blob/master/core/libs/backbone/backbone-0.9.1.js#L323 is what we're trying to emulate from the Backbone model, right? This is the event triggered by the wrapError method it leads to: https://github.com/uberVU/mozaic/blob/master/core/libs/backbone/backbone-0.9.1.js#L1341 . Does the 'error', originalModel, resp, options event signature match yours 'error', @, @, xhr.status?

I don't think we should have "this" twice, here's an example of what we expect from the contents of the error event: https://github.com/uberVU/mozaic/blob/master/core/widget/backbone_events.coffee#L35-L45

Also, we use this instead of just @ now, when we don't have any leading property. E.g. trigger(this) or trigger(@property). You can replace it elsewhere in the file as well, if you want to do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The call matches the trigger event signature
  2. params[0] expects the model and params[1] the collection. I have changed the returned collection on error event to be the given collection if model has no collection
  3. Thanks for the tip :) i've changed @ with this :)

@ovidiuch
Copy link
Contributor

Done, LGTM. Some of us made changes on the thehole repo w/out syncing, and others viceversa. Either way, this one is all set now, after merging please run the sync_mozaic script again and create a pull request in thehole to update Mozaic in our repo. Besides the constants.coffee file, all should be migrated back.

dianatatu added a commit that referenced this pull request Jun 24, 2013
@dianatatu dianatatu merged commit bec72be into master Jun 24, 2013
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.

None yet

2 participants