-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Stats: Trying to add The Last updated time to the StatsModule component #11035
Conversation
What's the reason for this? What user goal does it satisfy?
I think it's a good approach. We might review this later, but for now I'd 👍 Here's a visual reference gif: |
The reason for this is to let the user know how old is his data on long requests (websites with a lot of traffic for example), but I agree it's not that important since in most cases the request is quick enough. |
Ok. I'd leave this out for now. Adds visual clutter and I can't trace it back to a sensible need for it. |
Here is the related user feedback pKdGS-181-p2 ( not sure if the tampermonkey script works anymore ) So here are the user goals:
This card in old stats served as a navigation piece ( which users also miss being able to toggle with one click between sites ), but also the timestamp shown represents the time that the stats report was generated. The same navigation piece with the timestamp was shown on all stats pages, including summary pages. The data situation in calypso is a bit different as each stats module is updated independently - and even in some cases, multiple API calls are used to generate stats on some Insights blocks. The user goal here is to give them visibility to how current the data the data on-page is. I also failed to search prior to making the new issue, and here is further feedback on the original duplicate issue: #2533
That user goal also adds the need for an easy way to refresh within the desktop app. I believe that feedback was from Matt, but I'd have to dig through the p2 archives to verify. Alternate Idea |
Ok, understood. Thanks for the awesome recap and background. Very useful. I think a lean solution to that is this one: Note: the second line won't show up in the sticky header, just on the page. Unless we want that, so we might redesign the header a bit. Where btw we could also change the two arrows icon (which is click-to-refresh) to a pulsating dot of some kind if/when we're able to do auto-refresh of the whole page (like, a refresh every minute?). What do you think? |
Hey, tks @folletto This looks really good, but I have some technical concerns. Basically, each one of the panels has its own query, so if we were to show a global date like this "outside of the panels", we'll have to make choices. I suggest that we rely on the status of the last "statsVisits" query which corresponds to the bar chart query (the chart above) and we add a refresh mechanism to all stats queries. @timmyc you could have an opinion about this too |
Sounds reasonable to me, and would simplify the logic substantially. I think adding the timestamp to the stats "period" pages is a great first step - but we might want to eventually add a similar feature to summary and insight pages too. |
325aa35
to
cddfe9a
Compare
I updated the PR in cddfe9a to reflect the proposal above, but I have several remarks
|
Yep! That was just a design proposal in case we could do it. If we can't, just add the date, knowing that at any point we already know how to do also the manual update.
Since we are adding also a refresh mechanism – which is automatic right? – we can just show a pulsating dot on the side, like this: http://codepen.io/folletto/pen/KaQjLV
What do you mean here? |
The refresh mechanism is automatic on the pages showing the refresh date for now, we could generalize to all the stats pages.
Should we show this pulsating dot next to the last update date? I'm not sure where do you want to add this, should it be always visible? And if we already have something similar elsewhere, that could be helpful for consistency.
When you refresh the page, the last query date is not shown on the page because it's not persisted unlike the query results. If we were to change this and cache the date as well, we would probably need to show the full date (Day + Time) to avoid confusion with old dates. A user could visit the stats page today at 12 and come back tomorrow at the same time (or a week later). I was suggesting to show the date in a format like |
You mean then on the side of "Last update"? I would avoid that as it gets far too long, and it force the user to read it all. We should use natural language processing:
Ideally using the locale settings of the user. We should have already something like this as I think it's used elsewhere in Calypso... however, if for any reason we can't use it, we could have just a simple threshold that avoids locale:
|
Looking good!
|
I believe we use moment.js localized date format here. Oh - I see the timestamp itself:
If we always used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor questions, and overall looking really great.
@folletto had asked:
Always visible, maybe with a tooltip that when hovered says "Auto-refreshing every X minutes"
I'm not seeing a tooltip currently, do we want to have a tooltip?
Will users want to stop auto-refresh?
As I mentioned in the comments, perhaps in a follow-up PR I think we should consider not doing the heartbeat/refresh on periods that don't include today. Stats data from past periods shouldn't change so repeatedly requesting the same data over and over is kind of a waste of bandwidth.
Additionally, 1 minute seems a bit too eager to me. I think we should bump the refresh up to something like 3 minutes.
Here are a couple of gifs with refreshes in action. The first is for a page that doesn't have much data. Things get a bit jumpy looking to me when the loading placeholders get turned on/off:
Perhaps we can tweak the loading state toggle in <StatsModule />
to handle this a bit better.
On a stats page with data however, I really like the experience. The subtle pulsing of the header is nice:
@@ -142,7 +142,7 @@ class StatsGeochart extends Component { | |||
return ( | |||
<div> | |||
<div ref="chart" className={ classes } /> | |||
{ siteId && <QuerySiteStats statType={ statType } siteId={ siteId } query={ query } /> } | |||
{ siteId && <QuerySiteStats statType={ statType } siteId={ siteId } query={ query } heartbeat={ 60000 } /> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned during the hangout, maybe we can bump this up to 2 or 3 minutes. Eventually we might consider disabling the heartbeat on stat periods that are not "active", as-in the data will not change if the period does not include today.
{ queryDate && this.renderQueryDate() } | ||
</span> | ||
} | ||
{ showQueryDate && <div className="stats-date-picker__pulsing-dot" /> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra tab or spaces here?
@@ -75,8 +89,19 @@ class StatsDatePicker extends Component { | |||
return formattedDate; | |||
} | |||
|
|||
renderQueryDate() { | |||
const { queryDate, moment, translate } = this.props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to handle a case where queryDate
is null? Seems unlikely to happen but curious if we should still add an early return here if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is handled but on the calling side, I guess it's better here 👍
ff8d8b9
to
c140ddf
Compare
Several things in the last commit c140ddf :
|
@timmyc I preferred to avoid to use only Last update: 1 second ago (or similar and this is not updated until the next That's why I kept the |
Great suggestions above, and ace work!
Three things:
|
In 291e5f0 I've used a localized time format
This does not work well on other periods weeks, years and months I would rather keep the last update time or maybe drop this line entirely. |
const today = moment(); | ||
const date = moment( queryDate ); | ||
const isToday = today.isSame( date, 'day' ); | ||
return translate( 'Last update: %(time)s', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! I've found a possible matching string that has already been translated 79 times:
translate( 'Last updated: %s' )
ES Score: 7.32
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
Yes I agree, "or something similar"! I just think the time doesn't cut it, as it seems saying "this day was calculated only until $time", which isn't correct as the data is now completed for the full time period. I think that having some text, if we can find the right one, would be ace. Otherwise we can remove it, but I'd like to try to avoid the "jump" in the visual, as well as having a clear message about "this data is done". Ideas:
To be honest none of these feels great to me. Ideas welcome. :) |
Howdy, y'all! Davide asked me to take a look re: the language here -- a few thoughts from an editor: I don’t actually think you need text there, because the fact that the date listed will be in the past says pretty clearly that these stats are done being calculated. However, if you feel like you want something there, I’d probably go with something slightly more narrative/full sentence like "These are your complete stats for this day/week/month/year." Using a full sentence here (1) makes things extra clear and (2) the full sentence w/punctuation itself communicates “nothing is changing here, this is done." If listing a specific time is important, then I’d go with something like “Final calculation for this day/week/month/year completed at timestamp" -- but my choice would be the sentence above rather than the specific time listing. |
I tried to mock it up for reference: I think it works well, but I also agree it would make sense without. @youknowriad I'll let you choose since maybe code-wise one is easier than the other (i.e. considering also the sticky bar in the other PR). :) |
I think when the time period has passed, we just shouldn't render any sentence at all. I believe the date itself implies that the data is for a certain period. |
Thanks everyone for your reflexions on this. I think the majority wins here, let's drop this sentence. |
291e5f0
to
69b829d
Compare
I updated the PR with last remarks and rebased (the sticky behaviour), Let me know if there's any other concern :) |
@@ -37,6 +38,23 @@ export function rangeOfPeriod( period, date ) { | |||
} | |||
|
|||
/** | |||
* Returns true if is auto refreshing astats is allowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small typo here "Returns true if is auto refreshing of stats is allowed:
Testing out really well for me - love this addition! |
69b829d
to
f15f3c8
Compare
closes #10911
Since the stats data are now persisted in the local cache, when first hitting the stats page, we show some outdated stats while the query is fetching. So in this PR, I'm adding a blinking effect to the header of the stats module component while the query is fetching.
Also, I added a "time" icon with a title showing the elapsed time since the last stats request.
All this is experimental and needs a design review.
Testing instructions
stats/day/$site