Loading utility should enable success & error states

[mapsam]

Entered by friend Sam Bolstad while testing at his house :+1:

When results from Overpass API come back negative, the icon is still a green checkmark, which is a bit misleading.

[mapsam]

Expanding on this, the loading feature should have a success/error/info callback that dictates the icon/color - something like:

dc.util.loader = function(yes, typeClass) {
  var loader = $('<div>').addClass('dropchop-loader');

  if (yes) {
    $('body').addClass('dropchop-loading');
    $('body').append(loader);
  } else {
    $('body').removeClass('dropchop-loading');
    $('.dropchop-loader').addClass(typeClass).fadeOut(2000, function(){ // class that has icon built in
      $(this).remove();
    });
  }
};

[jczaplew]

I was thinking about this last night when I noticed a similar issue with the "Import file from a URL" feature. Perhaps a more Nodejs-y convention would be passing an optional error object, and displaying an appropriate loading state icon. Something like...

dc.util.loader = function(yes, error) {
    var stateClass = (error) ? 'loader-complete-error' : 'loader-complete';
    /*......*/

        $('.dropchop-loader).addClass(stateClass).fadeOut(....
}

Operationally, it would look like

// Toggle loading state
dc.util.loader(true);

// Remove loading state with a success
dc.util.loader(false);

// Remove loading state with an error
dc.util.loader(false, error);

Thoughts?

[jczaplew]

On a related note (this might be a separate issue) it might be best to handle the loading state on a method-by-method basis instead of universally through dc.util.xhr. Because it is handled directly in that function, there isn't a good way to handle situations like dc.ops.file['load-url'] where a valid request is made but an invalid GeoJSON object is returned.

By moving the loading state trigger out of dc.util.xhr there is more freedom to validate objects before informing the user that the process is complete.

[mapsam]

Excellent thoughts, @jczaplew. Definitely agree that taking the loader out of the xhr is a good call. What scenarios do we need unique icons/colors for?

• success
• error, mishandled response
• error, unsuccessful query (bad URL)
• successful query but "no data" (is this an error or more of an info notification?)

It seems like if we have these errors popping up, we'll want to notify what the error is. Do you think it would make sense to build that into the loader or just use dc.notify to pass a notification separately?

[jczaplew]

For notifying the user, I think that could be baked into the loader.

As for different icons/colors, it could possibly be generalized to

• success - everything went a-okay
• warning - something went right, something went wrong (good request/bad response or good request/no data, for example)
• error - a lot went wrong

How about something like

dc.util.loader = function(yes, errorMessage, errorType) {
    var loader = $('<div>').addClass('dropchop-loader');
    var stateClass = 'loader-complete';

    if (errorMessage) {
        // If there is an error type and it equal 'warning', show the warning symbol, otherwise default to error
        stateClass = (errorType && errorType === 'warning') ? 'loader-complete-warning' : 'loader-complete-error' ;
        dc.notify('error', errorMessage, 6000);
    }

    if (yes) {
      $('body').addClass('dropchop-loading');
      $('body').append(loader);
    } else {
      $('body').removeClass('dropchop-loading');
      $('.dropchop-loader').addClass(stateClass).fadeOut(2000, function(){
        $(this).remove();
      });
    }
  }

?

That way the loader function is generic and error messages can be unique to the method producing them.

[mapsam]

Looks good to me!

Semi-related: It seems important to have sticky notifications for errors so users can copy them without rushing before they automatically remove. Created a separate issue for this #193

[mapsam]

The _loader.scss file probably needs some reworking to take care of the extra classes - the animation should be removed from the base class as well, and instead we can have a unique "loading" class:

.dropchop-loader {
  &.loader-loading {}
  &.loader-error {}
  &.loader-warning {}
  &.loader-success {}
}