Add homepage / posts page setting functionality by bjtitus · Pull Request #14209 · wordpress-mobile/WordPress-iOS

bjtitus · 2020-05-28T03:47:01Z

This reuses the SelectPostViewController for selecting a post for the Homepage Settings functionality.

Note that this design is slightly different than the original issue. I wanted to reuse the same view controller we use for selecting a post for links and this view controller also includes search, which seems like a nice feature addition. If the grouped table style is necessary, I can adjust that style for this particular presentation.

Testing

All scenarios from #14169 should continue to work. The following represent several new scenarios to test:

Scenario 5

With a Jetpack or WordPress.com site, visit the Site Settings screen.
Tap Static Homepage if not already selected
Tap Homepage and select a page from the list
Ensure that the label on the Homepage row matches the page you selected
Tap back out to Settings and ensure that the Homepage Settings row now reads Static Homepage
On the web, visit the Customizer for your test site, and check that the Homepage Settings section shows the selected Homepage.

Scenario 6

On the web, visit the Customizer for your test site, and find the Homepage Settings section there. Change it to Static Homepage, and choose both a Homepage and a Posts Page. Don't forget to save your changes!
In the app, refresh your sites list and then navigate into your test site
Go to Site Settings, and check that the Homepage Settings row says "Static Homepage"
Check that the Homepage and Posts Page that you selected are shown in the Choose Pages section
Change the Homepage value to a different page
Reload the customizer on the web for the site, and check your new Homepage shows the newly selected page

Scenario 7

Build and run on your device
Navigate into Site Settings for your site, and tap Homepage Settings
Put your device into airplane mode
Try changing the Homepage value to a new page. A spinning activity indicator should how in the Homepage cell, the operation should fail (the setting should not change), and a notice should be displayed.

Scenario 8

This is to verify existing functionality which also used SelectPostViewController

Visit a post using the Classic Editor (existing Post > ... > "Switch to Classic Editor")
Insert a link using the keyboard toolbar
Tap "Link to Existing Content"
Ensure that selecting a post works and is linked properly.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2020-05-28T03:47:08Z

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

peril-wordpress-mobile · 2020-05-28T04:01:51Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

frosty · 2020-05-28T10:05:57Z

cc @SylvesterWilmott is this slight tweak to the posts / homepage picker okay with you? I think it's nice to have search in there, and a pragmatic way to get this shipped faster :)

frosty

Looking good, and works fine for me when testing it out. I left some small code cleanup suggestions.

One thing missing here from the designs is that ideally a page you've already selected for one of the settings shouldn't show up in the other setting's picker. We could address this in a followup PR if that's easier?

frosty · 2020-05-28T18:10:34Z

+            }
        }
-        if post.permaLink == selectedLink {
+        if isSelectedPost?(post) == true {


Could omit the == true here

The block is optional here so it needs something like == true or isSelectedPost?(post) ?? false.

Ahh, sorry!

frosty · 2020-05-28T18:30:26Z

+            homepageRow = ActivityIndicatorRow(title: Strings.homepage, animating: true, action: nil)
+        } else {
+            homepageRow = NavigationItemRow(title: Strings.homepage, detail: homepageTitle, action: { _ in
+                self.pushPageSelection(selectedPostID: self.blog?.homepagePageID) { [weak self] selected in


Could we extract the contents of this action to another method or something to make it a little clearer to follow? Also, should this self be weak?

frosty · 2020-05-28T18:30:57Z

    }

-    // MARK: - Updating Settings
+    open override func viewWillAppear(_ animated: Bool) {


Can we move this up below viewDidLoad?

frosty · 2020-05-28T18:33:38Z

+        let viewController = SelectPostViewController(blog: blog,
+                                                      isSelectedPost: { $0.postID?.intValue == selectedPostID },
+                                                      showsPostType: false,
+                                                      entityName: Page.entityName()) { (post) in


Can we keep the trailing closure name? I think it might just be a little easier to understand what the closure's for.

frosty · 2020-05-28T18:33:59Z

+            if let page = post as? Page {
+                completion(page)
+            }
+            self.navigationController?.popViewController(animated: true)


bjtitus · 2020-05-28T19:47:40Z

@frosty I totally missed that! It should be easy enough to add to this.

frosty

Looking good! Scenarios 5, 6, 7, and 8 are working as expected for me. I did find one issue related to bad / slow connectivity. It's easiest to see if you use Network Link Conditioner set to very bad network:

Go to the Homepage Settings screen, with it set to Static Homepage
Tap one of the rows and choose a page
While the loading indicator is still spinning for that row, go and select a page for the other row
Your second selection won't 'stick' – the cell will still show the original page name while the first one continues loading.

frosty · 2020-05-29T10:51:46Z

    }

-    func posts(for blog: Blog, containsTitle title: String, entityName: String? = nil) -> NSFetchedResultsController<AbstractPost> {
+    func posts(for blog: Blog, containsTitle title: String, entityName: String? = nil, excludingPostIDs excludedPostIDs: [Int] = []) -> NSFetchedResultsController<AbstractPost> {


I'd be tempted to switch around the order here so that containsTitle and excludingPostIDs are grouped together. So entityName should either go second or last. What do you think?

frosty · 2020-05-29T10:52:14Z

        if !title.trimmingCharacters(in: CharacterSet.whitespacesAndNewlines).isEmpty {
            compoundPredicates.append(NSPredicate(format: "postTitle contains[c] %@", title))
        }
+        compoundPredicates.append(NSPredicate(format: "NOT (postID IN %@)", excludedPostIDs))


Should we only add this if excludedPostIDs contains some items?

I thought about that, I figured it doesn't matter much if the array is empty but it's probably a theoretical performance improvement.

SylvesterWilmott · 2020-05-29T11:44:30Z

Looks great! Thanks @bjtitus

bjtitus · 2020-05-29T17:07:01Z

@frosty Fixed that bad network issue by just disabling selection on the table view. If that seems like a bad idea, I also considered allowing selection but immediately deselecting the cell and not performing the action (just so there's some intractability left for the user). I'm not quite sure which one seems less confusing – let me know what you think.

frosty

I think the current solution works fine – thanks for making that change! I've just added a code-level comment about a bug I spotted, and the last thing that I can see is that the activity indicator in the rows is currently super hard to see in dark mode. I'm surprised it doesn't change automatically – perhaps we need to tint it to a system color.

frosty · 2020-05-29T22:29:50Z

+            return ActivityIndicatorRow(title: title, animating: true, action: nil)
+        } else {
+            return NavigationItemRow(title: title, detail: detail, action: { [weak self] _ in
+                self?.showPageSelection(selectedPostID: self?.blog?.homepagePageID, hiddenPostID: self?.blog?.homepagePostsPageID, change: changeForPost)


We're always passing homepageID for the selected post, and postPageID for the hidden post ID here, so the picker shown the same content no matter which row you're tapping into. These should be switched for the Posts row.

frosty

Looks great! Activity indicator now visible in dark mode, and the correct page is hidden for each of posts / pages.

I'll go ahead and merge this ahead of the freeze on Monday.

bjtitus added 2 commits May 27, 2020 21:40

Add homepage / posts page setting functionality

b3f210e

Add release notes for Homepage Settings feature

d643511

bjtitus added [Type] Enhancement Site Settings labels May 28, 2020

bjtitus added this to the 15.0 milestone May 28, 2020

bjtitus requested a review from frosty May 28, 2020 03:47

bjtitus self-assigned this May 28, 2020

Filter to only pages in homepage selector

01a2367

frosty reviewed May 28, 2020

View reviewed changes

bjtitus added 4 commits May 28, 2020 15:03

Merge branch 'develop' into feature/homepage-settings-page-select

ea73ffb

Exclude already selected homepage settings pages

6ec1fe3

Address PR comments + some refactoring

7d7f45c

Fixed ACL issue

2e97acb

frosty reviewed May 29, 2020

View reviewed changes

Addressing PR comments + fixing network error issue

a3d34e2

bjtitus requested a review from frosty May 29, 2020 19:59

frosty reviewed May 29, 2020

View reviewed changes

Fix homepage settings selected post + activity color

172815f

bjtitus requested a review from frosty May 30, 2020 14:41

frosty approved these changes May 30, 2020

View reviewed changes

frosty merged commit 9f25996 into develop May 30, 2020

bjtitus deleted the feature/homepage-settings-page-select branch June 10, 2020 17:59

Uh oh!

Conversation

bjtitus commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Scenario 5

Scenario 6

Scenario 7

Scenario 8

Uh oh!

peril-wordpress-mobile Bot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile Bot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frosty commented May 28, 2020

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjtitus commented May 28, 2020

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvesterWilmott commented May 29, 2020

Uh oh!

bjtitus commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frosty left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bjtitus commented May 28, 2020 •

edited

Loading

peril-wordpress-mobile Bot commented May 28, 2020 •

edited

Loading

peril-wordpress-mobile Bot commented May 28, 2020 •

edited

Loading

bjtitus commented May 29, 2020 •

edited

Loading