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

Stats: Use site timezone #12159

Merged
merged 29 commits into from Jul 23, 2019

Conversation

@ScoutHarris
Copy link
Contributor

commented Jul 18, 2019

Fixes #12018

This changes all of Stats to use the site timezone instead of the device's timezone.

NOTE: The Post Stats endpoint does not honor the site timezone, but is in UTC. The result is that, depending on your local TZ, site TZ, and timing, the Post Stats and Latest Post Summary charts could be missing a day or have an extra day.

Corresponding WPKit PR: wordpress-mobile/WordPressKit-iOS#170

To test:

Test the following items on site with the timezone:

  • Before your local.
  • After your local (preferably a day ahead).
  • In UTC.
  • Same as your local.

Date Bar:

  • If your timezone differs from the site, a 'Site timezone' label will appear in the date bar showing the offset from UTC.
  • If your timezone is the same as the site, the 'Site timezone' label will not appear.

Periods:

  • Verify changing the date via the bar works as expected.
  • Verify changing the date via the chart works as expected.
  • Compare data with Calypso. Especially Days as that is the one most affected.

Insights:

  • Verify Today shows today's stats.
  • Go to This Year > View more. Verify the date bar behaves as expected.
  • Verify the Latest Post Summary chart is correct (taking the note above into consideration).

Post Stats:
Access Post Stats via Posts and Pages and Latest Post Summary.

  • Verify the date bar behaves as expected.
  • Verify the chart behaves as expected.
  • Verify the chart is correct (taking the note above into consideration).

Timezone:

  • Change site timezone via site settings. Verify the date bar is updated accordingly.

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

ScoutHarris added some commits Jul 12, 2019

When site timezone is changed via site settings, update SiteStatsInfo…
…rmation timezone so it's updated on the date bar.

@ScoutHarris ScoutHarris added this to the 13.0 milestone Jul 18, 2019

@ScoutHarris ScoutHarris self-assigned this Jul 18, 2019

@ScoutHarris ScoutHarris added this to In progress in Full Stats Refresh via automation Jul 18, 2019

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@SylvesterWilmott - screenshots:

iphone

ipad

@ScoutHarris ScoutHarris referenced this pull request Jul 18, 2019

Merged

Stats: Use site timezone for Published posts. #170

0 of 1 task complete
@frosty

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Thanks for all your work getting this together @ScoutHarris! I haven't had much of a chance to test yet, but I just had a few thoughts regarding the style of the date bar having seen it in action (partly for @SylvesterWilmott):

  • I think we should reduce the size of the timezone label text – maybe subhead or footnote?
  • I think the standard way of showing UTC offsets would be with no spaces either side of the + / - – e.g. UTC+7.
  • However, we also need to account for locations that have a 30 or 45 minute offset. Possibly +0730 or +07:30 in those cases?

Full Stats Refresh automation moved this from In progress to Reviewer approved Jul 19, 2019

@danielebogo
Copy link
Contributor

left a comment

LGTM! Well done with it!
The test went well and I compared the data with Calypso. Everything still works correctly!
:shipit:

@SylvesterWilmott

This comment has been minimized.

Copy link

commented Jul 19, 2019

I think we should reduce the size of the timezone label text – maybe subhead or footnote?

It should be subhead in the screenshots above. Is the issue in how large the bar is because it is slightly larger than it should be. @ScoutHarris currently the bar is at 68pt, can we reduce it to 60pt?

I think the standard way of showing UTC offsets would be with no spaces either side of the + / - – e.g. UTC+7.

Looking into this, that is correct. The correct formatting would be UTC+1 UTC-2 etc

However, we also need to account for locations that have a 30 or 45 minute offset. Possibly +0730 or +07:30 in those cases?

It looks like there's a few standard formats for this. For readability let's go with:

UTC+07:30

@frosty

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

It should be subhead in the screenshots above. Is the issue in how large the bar is because it is slightly larger than it should be. @ScoutHarris currently the bar is at 68pt, can we reduce it to 60pt?

I think it's callout, but it doesn't look much smaller than the main date text, just a lighter weight. It felt like it could be smaller?

Screenshot 2019-07-19 at 13 35 22

Thanks for the clarification on the other items :)

@danielebogo danielebogo referenced this pull request Jul 19, 2019

Merged

[Stats Refresh] Improve Insights Store #12169

0 of 1 task complete
@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@frosty , @SylvesterWilmott - To get it to fit in 60pt I had to squinch it a bit. Meaning:

  • Remove 1pt from top and bottom margins (from 12 to 11).
  • Reduce space between date and timezone labels to 1pt (was 2).
  • Use the footnote font (was callout).

tz_1

tz_2

tz_3

(@frosty - thank you for noticing the minutes issue.)

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@danielebogo - Since the timezone format has been changed, I closed #12163 and updated the accessibilityLabel here.

@ScoutHarris ScoutHarris requested review from danielebogo and frosty Jul 19, 2019

@danielebogo

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Hey @ScoutHarris ! LGTM!
I tested it again and everything looks great! Even the accessibility label.
Little note about the conflicts with SiteStatsInsightsTableViewController: keep only let year = viewModel?.annualInsightsYear() from develop.
:shipit:

@SylvesterWilmott

This comment has been minimized.

Copy link

commented Jul 22, 2019

To get it to fit in 60pt I had to squinch it a bit.

Does it still look the same as before when the new label isn't included? Ie 44pt without the new label and 60pt with it. I'm not 100% on the new text size, could you include full screenshots here of iPhone X and iPad please?

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

Hey @SylvesterWilmott .

Does it still look the same as before when the new label isn't included? Ie 44pt without the new label and 60pt with it.

Yes.

include full screenshots

Here ya go.

iphone_date

iphone_with_tz

ipad_date

ipad_with_tz

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@SylvesterWilmott this is a pretty significant change. So I'm going to go ahead and merge it. If there are any required changes to the timezone display, just let me know and I'll do them asap.

@ScoutHarris ScoutHarris merged commit c195105 into develop Jul 23, 2019

3 checks passed

Hound No violations found. Woof!
Peril All green. Congrats.
Details
ci/circleci: build_and_test Your tests passed on CircleCI!
Details

Full Stats Refresh automation moved this from Reviewer approved to Done Jul 23, 2019

@ScoutHarris ScoutHarris deleted the issue/12018-stats_timezone branch Jul 23, 2019

@SylvesterWilmott

This comment has been minimized.

Copy link

commented Jul 24, 2019

this is a pretty significant change. So I'm going to go ahead and merge it. If there are any required changes to the timezone display, just let me know and I'll do them asap.

Sure. The only change is really to move the text size of the site timezone label back to what we had before but to retain the 60pt height of the bar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.