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 Widgets] Update current Today widget to new design #12893

Merged
merged 23 commits into from Nov 12, 2019

Conversation

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Nov 6, 2019

Ref #12849

This updates the current Today widget to the new design. Specifically, the "compact" view, which is the only one currently displayed.

Expanded view will be addressed in a future PR. To facilitate that, I've included some things in this PR that may seem unnecessary, but will be used later:

  • Likes and Comments are now saved to the plist, but aren't displayed yet.
  • The data (Views and Visitors) is inserted with a TwoColumnRow view. This will allow easily adding another data "row" when the view is expanded.
  • initRows only inits one row. But it will be multiple when expanded, hence the plural instead of singular. Stay tuned!

To test:


  • Start with a fresh install.
  • Add the WP widget to the Today view.
  • Verify the unconfigured view looks like this:

unconfigured_light

unconfigured_dark


  • Log into the app and go to Stats.
  • Select 'Today' in the nav bar and select 'Use this site'.
  • Return to the widget.
  • Verify the view looks like this. Things to note:
    • The widget display name is now "WORDPRESS TODAY".
    • The site url is displayed at the bottom.

configured_light

configured_dark


Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
ScoutHarris added 12 commits Nov 5, 2019
Moving constants to structs, don't force unwrap, renaming vars.
Essentially a modified version of StatsTwoColumnRow used in Insights.
@ScoutHarris ScoutHarris added this to the 13.7 milestone Nov 6, 2019
@ScoutHarris ScoutHarris requested review from frosty and danielebogo Nov 6, 2019
@ScoutHarris ScoutHarris self-assigned this Nov 6, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 6, 2019

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 6, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 6, 2019

Hey @osullivanchris . Here are some hi res screenshots for you. I'm not sure about the dark mode button background color (the "Open WordPress" button). Let me know if you'd like any changes.

Light Mode:
light_unconfigured

light_configured

Dark Mode:
dark_unconfigured

dark_configured

Copy link
Contributor

danielebogo left a comment

Hey @ScoutHarris the widget looks much better! Well done with it! I also tested it in Dark Mode and looks perfect (maybe the button to open WP is too dark but I'll leave it for @osullivanchris )!
I left some nitpick, other than that :shipit:

@IBOutlet private weak var leftItemLabel: UILabel!
@IBOutlet private weak var leftDataLabel: UILabel!
@IBOutlet private weak var rightItemLabel: UILabel!
@IBOutlet private weak var rightDataLabel: UILabel!
Comment on lines 7 to 10

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

No need to set these as weak 😊

@IBOutlet private weak var unconfiguredView: UIStackView!
@IBOutlet private weak var configureLabel: UILabel!
@IBOutlet private weak var configureButton: UIButton!

@IBOutlet private weak var configuredView: UIStackView!
@IBOutlet private weak var rowsStackView: UIStackView!
@IBOutlet private weak var separatorLine: UIView!
@IBOutlet private weak var siteNameLabel: UILabel!
@IBOutlet private weak var siteUrlLabel: UILabel!
Comment on lines 11 to 19

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

Can me make these strong?

}

func initRows() {
guard let row = Bundle.main.loadNibNamed(Constants.rowNibName, owner: nil, options: nil)?.first as? TwoColumnRow else {

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

You can use loadRootViewFromNib from Bundle+LoadFromNib

This comment has been minimized.

Copy link
@ScoutHarris

ScoutHarris Nov 7, 2019

Author Contributor

Well, I'd have to include that file in the widget's target. Which I could do. But I don't see a reason to add another file to the widget footprint for a convenience method when this works fine.

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

Ah got it! Was just to don't have duplication since there's an extension for it, but it's ok if you don't want to include it.

let numberFormatter = NumberFormatter()
numberFormatter.numberStyle = .decimal
Comment on lines 216 to 217

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

Can the formatter be a let stored property with closure? Just to avoid to create a formatter every time the method is invoked.

@@ -186,4 +232,18 @@ private extension TodayViewController {
return StatsServiceRemoteV2(wordPressComRestApi: wpApi, siteID: siteID.intValue, siteTimezone: timeZone)
}

struct LocalizedText {

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

Nitpick, enum here is safer than struct

static let views = NSLocalizedString("Views", comment: "Stats Views Label")
}

struct Constants {

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 7, 2019

Contributor

Here too 😊

Copy link
Contributor

frosty left a comment

Looking great!

Can I suggest we change the name to "WordPress – Today"? If we just call it Today then it's listed under Today in the widgets list, when as a user I'd expect to find it under WordPress. Also, when we have multiple widgets, they'll all be sorted together if we use a prefix like this. I've noticed other apps do the same.

IMG_F0F612387B9B-1

One other query for @osullivanchris: testing on my 11 Pro and iPhone X, and there's really no room for the .wordpress.com part of the site URL which makes me question the usefulness of having it there. I wonder if the view is compact if we should remove either the title or the URL and allow more space for whichever we keep. Any thoughts?

IMG_6FFC8C79DC94-1

@osullivanchris

This comment has been minimized.

Copy link

osullivanchris commented Nov 7, 2019

@ScoutHarris Hey! Again, this looks to me like its using a system material and should be a semi transparent background rather than a hard grey colour.

In my files I found that using the 'thin' system material approximated the background of the box, and ultrathin on top of it for the button background, seemed to work well.

I am not sure if the naming convention is the same in the design components they provide as in the components/code. But what you have for the box background looks right, and I've gone one level lighter/thinner for the button background.

Here's how it looks on black like in Sylvester's mock, and on a phone background.

Screenshot 2019-11-07 at 17 32 22

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 7, 2019

@frosty @osullivanchris

there's really no room for the .wordpress.com part of the site URL which makes me question the usefulness of having it there. I wonder if the view is compact if we should remove either the title or the URL and allow more space for whichever we keep. Any thoughts?

Just a couple comments.

I wonder if the view is compact

Compact vs expanded only affects the height, so this won't help.

remove either the title or the URL

It is possible for a site to not have a title. If we elect to show only one value, maybe it should be the url so something is shown? Or we could show the site name if it exists. If not, fallback to the url.

@osullivanchris

This comment has been minimized.

Copy link

osullivanchris commented Nov 7, 2019

Sorry it just re-loaded when I added my comment and I saw @frosty's. I don't think its a big deal to remove one of those, I agree its better not to truncate especially twice.

The url seems a safe bet if there is sometimes not a title. The URL is also technically the thing that people are visiting, so its rational to me to have that one in there.

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 7, 2019

@osullivanchris Can I make a suggestion?

There really is no need to have a button. It doesn't actually do anything as the entire widget is tappable. Maybe it would look better if we just remove it, and do something like this?

no_button

@frosty

This comment has been minimized.

Copy link
Contributor

frosty commented Nov 8, 2019

There really is no need to have a button. It doesn't actually do anything as the entire widget is tappable.

That's definitely true, but I think having the visible button there makes the whole thing look more tappable?

@danielebogo

This comment has been minimized.

Copy link
Contributor

danielebogo commented Nov 8, 2019

@osullivanchris @ScoutHarris @frosty Should the button follow one of these style? Or this is a specific/different case?
buttons

@osullivanchris

This comment has been minimized.

Copy link

osullivanchris commented Nov 8, 2019

@osullivanchris @ScoutHarris @frosty Should the button follow one of these style? Or this is a specific/different case?

I think this is definitely a different case, the widgets all look totally part of the native UI and having our own button styles would look out of place there

There really is no need to have a button. It doesn't actually do anything as the entire widget is tappable.

That's definitely true, but I think having the visible button there makes the whole thing look more tappable?

All the other widgets I've checked don't really have buttons. Just links/content. What about a divider? I think it makes it clear that there is an action there without needing the background. I also tried two further explorations with the text.

In the second option, I was thinking its a bit weird to say 'Tap' on a touch device, a bit redundant. And the third option - I was wondering if we are deep linking to stats here or just opening the app? If we can deep link, perhaps we can make it clear that we're gonna help show you where to set up the widget? (if we're doing this maybe get a quick copy review).

Let me know if you agree!

widget

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 8, 2019

I was wondering if we are deep linking to stats here or just opening the app?

@osullivanchris we're just opening the app. Deep linking into stats requires a Site ID in order to know what stats to display. We don't have that yet, hence this view. 😄

It sounds like we'll be going with the 2nd option.

Screen Shot 2019-11-08 at 11 07 57 AM

Thank you all for your feedback!

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 8, 2019

@osullivanchris @danielebogo @frosty I believe I've covered all the feedback. I've updated the description with new screenshots. Ready for another review please! Thanks!

@ScoutHarris ScoutHarris requested review from danielebogo and frosty Nov 8, 2019
@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 8, 2019

Hey @osullivanchris . Final hi res screenshots for you.

Light Mode:

unconfigured_light

configured_light

Dark Mode:

unconfigured_dark

configured_dark

@osullivanchris

This comment has been minimized.

Copy link

osullivanchris commented Nov 8, 2019

@ScoutHarris thanks! as its a final screenshot I just did a double check against the mocks :)

The only two things I see different are

  • the mocks have a header background which the screenshots dont. However, again comparing to other widgets I see none with headers. So perhaps that's an outdated style. Let's leave it per the build.
  • The dividers in the screenshots are full-width. In the mocks, for the ones with stats highlights (2-4 stats) the dividers are padded in from the left to align with the left-aligned content. Note that its just for those cards. Well this is a detail for sure anyway - if its a quick fix lets change - if not its definitely not a blocker.

Thanks for the ping!

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 8, 2019

Hey @osullivanchris .

the mocks have a header background which the screenshots dont. However, again comparing to other widgets I see none with headers. So perhaps that's an outdated style. Let's leave it per the build.

You are correct. There is no way to customize the header - it's iOS being iOS. iOS12 does look more like the mocks, but iOS13 is much more transparent.

The dividers in the screenshots are full-width. In the mocks, for the ones with stats highlights (2-4 stats) the dividers are padded in from the left to align with the left-aligned content. Note that its just for those cards. Well this is a detail for sure anyway - if its a quick fix lets change - if not its definitely not a blocker.

I was referencing Zeplin, which has them full width. Although I will say, left indenting looks a little odd... Anyway, if you want it so, I'd be happy to do so in my next PR of that's OK.

Thanks!

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 8, 2019

I'd be happy to do so in my next PR of that's OK.

And I just realized you have no idea what's next. 😄 I'm working on expanding the widget, so I'm modifying the view anyway. I can make any adjustments with that.

Copy link
Contributor

danielebogo left a comment

LGTM! :shipit:

@ScoutHarris ScoutHarris dismissed frosty’s stale review Nov 12, 2019

Requested changes made. Dismissing so I can merge.

@ScoutHarris ScoutHarris merged commit 9870c60 into develop Nov 12, 2019
7 checks passed
7 checks passed
Hound No violations found. Woof!
Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: Build Tests Your tests passed on CircleCI!
Details
ci/circleci: Installable Build/Hold Your job is on hold on CircleCI!
Details
ci/circleci: UI Tests (iPad Air 3rd generation) Your tests passed on CircleCI!
Details
ci/circleci: UI Tests (iPhone 11) Your tests passed on CircleCI!
Details
ci/circleci: Unit Tests Your tests passed on CircleCI!
Details
@ScoutHarris ScoutHarris deleted the feature/12849-today_widget_new_design branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.