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] Allow Today widget to expand/collapse #12946

Merged
merged 10 commits into from Nov 15, 2019

Conversation

@ScoutHarris
Copy link
Contributor

ScoutHarris commented Nov 14, 2019

Fixes #12849

This allows the Today widget to expand. When expanded, Likes and Comments are displayed.

The view now uses a tableView with custom cells to display the widget.

  • If unconfigured, a WidgetUnconfiguredCell is displayed.
  • If configured:
    • A WidgetTwoColumnCell is displayed for each row.
    • When the widget display mode is changed, the Likes and Comments row is inserted/deleted accordingly.
    • A WidgetFooterView displays the site url in the table footer.

To test:


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

unconfigured_light


  • Log into the app and go to Stats.
  • Select 'Today' in the nav bar and select 'Use this site'.
  • Return to the widget.
  • Verify it now contains data for the selected site.
  • Verify expanding/collapsing toggles Likes & Comments.

expandy


  • Go back to the app and log out.
  • Return to the widget.
  • Verify the unconfigured view is displayed correctly.

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 ScoutHarris added this to the 13.7 milestone Nov 14, 2019
@ScoutHarris ScoutHarris self-assigned this Nov 14, 2019
@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Nov 14, 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 14, 2019

Fails
🚫

Danger failed to run /app/danger-0.1tnucadudm8.ts.

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

Error TypeError

Cannot read property 'head' of undefined
TypeError: Cannot read property 'head' of undefined
    at Object.exports.default (/app/danger-0.1tnucadudm8.ts:31:46)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

26|     const hasModifiedAppStoreStrings = modifiedFiles.some(f => f.includes("Resources/AppStoreStrings.po"));
27| 
28|     if (hasModifiedReleaseNotes && !hasModifiedAppStoreStrings) {
29|         warn("The AppStoreStrings.po file must be updated any time changes are made to release notes");
30|     }
------------------------------------------------^
31| 
32|     // Changes to Resources/en.lproj/Localizable.strings should only be made on release branches since this file is
33|     // generated by our scripts
34|     const hasModifiedStrings = modifiedFiles.some(f => f.endsWith("Resources/en.lproj/Localizable.strings"));

Generated by 🚫 dangerJS

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 14, 2019

@osullivanchris - for your viewing pleasure.

To note, in iOS13 the Show More / Show Less labels have been replaced with a chevron.

Light Mode:

unconfigured_light

configured_compact_light

configured_expand_light

Dark Mode:

unconfigured_dark

configured_compact_dark

configured_expand_dark

Copy link

osullivanchris left a comment

LGTM!

Copy link
Contributor

frosty left a comment

Looks fantastic! Code looks great too :)

I just spotted one issue, when installing this as an update over an existing installation of the app that was using the old style widget:

IMG_0353D1E4425B-1

The site URL doesn't get automatically populated, so there's an empty space at the bottom ^

Copy link
Contributor

danielebogo left a comment

This looks great! I like it! It works really well! I tried it with different sites and it worked well!
I left some comment about the outlets and weak but other than that :shipit:

@IBOutlet private weak var separatorLine: UIView!
@IBOutlet private weak var siteUrlLabel: UILabel!
@IBOutlet private weak var backgroundColorView: UIView!
Comment on lines 9 to 11

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 14, 2019

Contributor

You can avoid the weak here 😊

@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 9 to 12

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 14, 2019

Contributor

Here too, you can remove the weak!

@IBOutlet private weak var configureLabel: UILabel!
@IBOutlet private weak var separatorLine: UIView!
@IBOutlet private weak var openWordPressLabel: UILabel!
Comment on lines 9 to 11

This comment has been minimized.

Copy link
@danielebogo

danielebogo Nov 14, 2019

Contributor

Here too!

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 14, 2019

The site URL doesn't get automatically populated, so there's an empty space at the bottom

Nice catch @frosty ! Thanks!

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 14, 2019

The footer will no longer be displayed if we don't have the site url. The minor side effect is the compact view has a bit of extra space at the bottom because the widget has a minimum compact height. I can't really change the stat row height/position to accommodate as that will cause the view to "jump" when expanded. I don't think this is a huge deal since the site url will get populated once the user accesses the app, causing the footer to be displayed.

no_footer_compact

no_footer_expanded

@ScoutHarris

This comment has been minimized.

Copy link
Contributor Author

ScoutHarris commented Nov 14, 2019

@danielebogo @frosty ready for another go please.

@ScoutHarris ScoutHarris requested review from danielebogo and frosty Nov 14, 2019
Copy link
Contributor

danielebogo left a comment

LGTM! :shipit:

@frosty
frosty approved these changes Nov 15, 2019
Copy link
Contributor

frosty left a comment

:shipit:

@ScoutHarris ScoutHarris merged commit 9e1ad3d into develop Nov 15, 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_expandable branch Nov 15, 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.