Skip to content

Fix loading text if naturalEvents is not set - #628

Merged
localjo merged 6 commits into
nasa-gibs:module-loadersfrom
EXWEXs:fix-loading
Jan 22, 2018
Merged

Fix loading text if naturalEvents is not set#628
localjo merged 6 commits into
nasa-gibs:module-loadersfrom
EXWEXs:fix-loading

Conversation

@PacoDu

@PacoDu PacoDu commented Dec 5, 2017

Copy link
Copy Markdown
Contributor

Fixes #627 .

Changes in this pull request:

  • Split div wv-events into wv-events and wv-loading for consistency
  • Added loading hide() after sidebar rendering

@nasa-gibs/worldview

@PacoDu
PacoDu changed the base branch from master to development December 5, 2017 12:25
Comment thread web/index.html Outdated
<div id="products"></div>
<div id="wv-events">Loading...</div>
<div id="wv-events"></div>
<div id="wv-loading">Loading...</div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps this div should go below #wv-data, since "Loading..." is actually indicating that any part of the sidebar is in a loading state and, it contains less important information than what is in the data tab. Otherwise, this looks good to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes I agree, I'll push that in a minute

@localjo

localjo commented Dec 5, 2017

Copy link
Copy Markdown
Contributor

I originally added the "Loading..." text to fix an issue; when switching to the events tab, there is a delay while events information loads and the tab was empty with no indication that anything was loading. On some connections, it would take several seconds for anything to display in the tab, which made it seem like there were no events and would lead to users switching tabs again before anything loaded. You can reproduce that issue by setting your dev tools to emulate a slow connection, disabling cache, reloading the page and then clicking on the events tab. On master you should see "Loading..." until the events load. This PR removes that text from the events tab, so it reintroduces the original problem.

@PacoDu

PacoDu commented Dec 5, 2017

Copy link
Copy Markdown
Contributor Author

Oh ok I see, so should I just add an else condition after

if (config.features.dataDownload) {
var $downloadTab = $('<li></li>')
.addClass('layerPicker')
.addClass('third')
.attr('data-tab', 'download');
var $downloadLink = $('<a></a>')
.attr('href', '#wv-data')
.addClass('tab')
.html('<i class=\'productsIcon selected icon-download\' title=\'Data\'></i> Data');
$downloadTab.append($downloadLink);
$tabs.append($downloadTab);
}

Something like:

else{
   $('#wv-events').hide()
}

@Benjaki2

Benjaki2 commented Dec 5, 2017

Copy link
Copy Markdown
Contributor

@PacoDu I think that condition could go here for a quick fix: https://github.com/nasa-gibs/worldview/blob/master/web/js/wv.main.js#L147-L149

But it would probably be better to take the Loading... text out of the index.html and actually append that when wv.naturalEvents.ui is initiated. What do you think?

@PacoDu

PacoDu commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

@Benjaki2 Yes the code would be more consistent. I don't have time now but I'll take a look tomorrow.

@PacoDu

PacoDu commented Jan 19, 2018

Copy link
Copy Markdown
Contributor Author

Well, sorry for the delay :).
I've moved the Loading string to naturalEvents init, this Fixes #627 and still display "Loading..." on NaturalEvent tab for slow connections (tested with 3G network settings on chrome).

@Benjaki2

Copy link
Copy Markdown
Contributor

@PacoDu We are in the process of merging and releasing #503 which should be on Production Monday (if all goes as planned 🤞 ). We will try to get this merged in right after. Thanks for the contribution!

@ZachTRice
ZachTRice requested a review from localjo January 19, 2018 21:23

@localjo localjo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. We may have a little trouble merging due to other changes that might conflict.


var init = function () {
// Display loading information for user feedback on slow network
$('#wv-events').text('Loading...');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file has changed in the module-loaders branch. Let's try to change the target of this pull request to that branch, and manually adjust it if necessary.

@localjo
localjo changed the base branch from development to module-loaders January 22, 2018 20:58
@localjo
localjo merged commit 2e32157 into nasa-gibs:module-loaders Jan 22, 2018
@minniewong minniewong added this to the Jan Release 2 milestone Jan 29, 2018
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.

5 participants