Closed laurenwalker closed 5 years ago
Hey @laurenwalker - thanks for writing up this issue.
So, regarding 1 - Dave, Chris and I looked into the error and turns out there was some issue with the Multi-processing implementation of the same. We eventually ended up deciding to keep the processing serial for now, till we come up with a better solution.
With serial processing on the back-end, the XHR request might take a while to load the metrics. So, in this commit, I've extended the XHR request timeout to the metrics service to 50s
.
Regarding 2 - yes, the icon-spinner
keeps on spinning when the call fails which is wrong. So, I made a few changes in the above commit. Here's what the UI looks like -
I have a small doubt regarding the implementation:
In the MetricView
, I have -
...
// waiting for the fetch() call to succeed.
this.listenTo(this.model, "sync", this.renderResults);
// in case when there is an error for the fetch call.
this.listenTo(this.model, "error", this.renderError);
Do we have to make sure that the MetricView
stops listening for sync
event of the MetricsModel
in case of the error
event?
Thanks Rushi! Some thoughts:
I would suggest changing the tooltip text to: The number of citations/downloads/views could not be retrieved right now.
We haven't been using the word metric
in the UI since it's a bit jargony.
I think we should also gray out the buttons when the request fails, the same way we do when the count is 0.
After looking at the question mark, I think I like the exclamation sign icon better: https://fontawesome.com/v3.2.1/icon/exclamation-sign
No, we do not have to stop listening to the sync event. It doesn't really hurt anything to keep that listener there.
Hey Lauren - I appreciate you taking time and penning down these suggestions.
Yes, I agree to the first point. It would be good to remove metric
jargon from the tool-tip.
We don't really gray out the metric buttons right now. I remember Matt suggested - that we require the buttons to be enabled because we have the functionality to navigate modals. But, now that I think of it, we should just keep the buttons enabled if either one of the three is non-zero
. So, we would gray-out (and disable button) in two scenarios: if all the three metrics are zeros; and if the metrics request call fails. Please let me know if this sounds reasonable.
Okay, I'll place in the exclamation sign
when the call fails.
And, it is good not to explicitly have to stop listening. Makes things easier!
I'll go ahead and start working on these changes. Thanks again!
Reference commit: https://github.com/NCEAS/metacatui/commit/684c273a7ef7543cfe01e40b25c2f53a81a8dc11
Disabled the icons on metrics-service failure:
Sometimes the call to the metrics service on the MetadataView fails due to a time out error.
Example dataset where this happened at least twice: https://knb.ecoinformatics.org/view/urn:uuid:0beca8b9-7fcb-468f-9118-2bcc9f641f90
There are two issues here: