Issue 19051 - Suggestions list disappear after expanding/collapsing the reply input by igoriols · Pull Request #19248 · wordpress-mobile/WordPress-iOS

igoriols · 2022-08-26T02:19:47Z

Closes #19051

Description

In this PR, I'm addressing the requested change to keep the suggestions list visible when expanding/collapsing the reply text field. If the user inserts a @ the suggestions appear. And if the user clicks on the ^ button, it should be visible when the text area is expanded. Following the same idea, if the text area is expanded and the suggestions list is visible, it should remain visible when the user clicks on the ⌄ button.

Details

I created FullScreenCommentReplyViewModel to extract business logic from FullScreenCommentReplyViewController and make it easy to test the visibility of the suggestions list.
I added a new parameter on FullScreenCommentReplyViewController.enableSuggestions representing the last search string provided by the user, and it is being called before FullScreenCommentReplyViewController is presented.
I changed FullScreenCommentReplyViewController to display the suggestions list if the search text was provided. The suggestions list is shown or not in viewDidAppear using the search text provided. The suggestions list remains hidden if the search text is nil or empty.
I modified FullScreenCommentReplyViewController.onExitFullscreen to have one more string as the last parameter. It represents the last search text inserted by the user when the text input area is expanded.
When the user exits FullScreenCommentReplyViewController, onExitFullscreen completion block is called with the last search text provided. If the user press Reply there is no need to keep the list visible, so I'm sending nil in the completion block.
I created a new method in ReplyTextViewDelegate which is being called when FullScreenCommentReplyViewController is closed. The implementation receives the last search text provided by the user, and displays the suggestions list.
I'm calling the new delegate method on every view controller that calls FullScreenCommentReplyViewController enabling the suggestions list visibility. So, every view controller that conforms with willEnterFullScreen method is conforming with the new didExitFullScreen method.

Preview

Testing Instructions

N.B: The user-mentions suggestions feature is not enabled in self-hosted sites ( wordpress.org ).

Scenario 1 - Suggestions list remains visible when the text area is expanded

Tap the "My Site" tab.
Tap on "Menu".
Tap on "Comments" row.
Tap any comment.
Tap "Reply" button.
Type "@" and you should see the user-mentions suggestions list.
Click on the ^ button.
Check if the suggestions list remains visible.

Scenario 2 - Suggestions list remains visible when the text area is collapsed

Tap the "My Site" tab.
Tap on "Menu".
Tap on "Comments" row.
Tap any comment.
Tap "Reply" button.
Type "@" and you should see the user-mentions suggestions list.
Click on the ^ button.
Erase the text to make the suggestions list disappear.
Make it appear again by typing "@".
Click on the ⌄ button to collapse the text area.
Check if the suggestions list remains visible.

There are other scenarios to display the suggestions list described in #18979.

Regression Notes

Potential unintended areas of impact
Unfortunately I couldn't create unit tests for every view controller conforming with the new didExitFullScreen delegate method: CommentDetailViewController, NotificationDetailsViewController and ReaderCommentsViewController.
What I did to test those areas of impact (or what existing automated tests I relied on)
I've manually tested all the scenarios where didExitFullScreen is called.
What automated tests I added (or what prevented me from doing so)
I added FullScreenCommentReplyViewModelTests and I added new tests in FullScreenCommentReplyViewControllerTests to check the visibility of the suggestions list.
I couldn't add new tests for CommentDetailViewController, NotificationDetailsViewController and ReaderCommentsViewController to ensure the suggestions list is displayed because those view controllers are not unit-testable.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding unit tests for my changes.
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.

…odel

…ntReplyViewController and make suggestions visible if needed

hassaanelgarem · 2022-08-29T00:33:08Z

@igoriols The first time the input field is expanded, the issue is fixed. However, if I expand the list again the list disappears.

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-08-29.at.02.26.44.mp4

igoriols · 2022-08-29T12:32:07Z

@igoriols The first time the input field is expanded, the issue is fixed. However, if I expand the list again the list disappears.
Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-08-29.at.02.26.44.mp4

Sorry, but I couldn't reproduce the behavior you identified. Here is what I found:

Could you please share more details about how you identified what you have found?
How did you access the screen to reply to the comment?

Thanks in advance.

hassaanelgarem · 2022-08-29T12:51:55Z

Could you please share more details about how you identified what you have found?

@igoriols Sure!

Make sure to have a comment on a post with likes
Install the app
Go to the notifications tab
Tap on "Someone liked your comment" notification
Tap on the comment
This opens up the comments view
Type in the reply field "@" followed by any prefix
Expand the reply field
Collapse the reply field
Expand the reply field
Notice that the suggestions list is not visible

I think it's reproducible from any type of notification not just "Someone liked your comment"

igoriols · 2022-08-29T17:53:15Z

Could you please share more details about how you identified what you have found?

@igoriols Sure!
1. Make sure to have a comment on a post with likes

2. Install the app

3. Go to the notifications tab

4. Tap on "Someone liked your comment" notification

5. Tap on the comment

6. This opens up the comments view

7. Type in the reply field "@" followed by any prefix

8. Expand the reply field

9. Collapse the reply field

10. Expand the reply field

11. Notice that the suggestions list is not visible
I think it's reproducible from any type of notification not just "Someone liked your comment"

Thank you for the detailed steps. Sorry, but I still can't reproduce it yet. 😕

I also tried to reproduce it with other types of notifications, but I couldn't reproduce it yet. The new method I created is being called every time the suggestions list visibility is enabled. So, it should work by expanding/collapsing the reply field.

One thing I noticed is that sometimes the UI hangs during the search. Maybe a possible solution is to extract the search execution to another thread.

The behavior you found is always happening?
Could you please check it again?

hassaanelgarem

The behavior you found is always happening?
Could you please check it again?

I tested again on a physical device and the issue is not happening. Looks like a simulator bug 🤦‍♂️

WordPress/Classes/ViewRelated/Comments/CommentDetailViewController.swift

hassaanelgarem · 2022-08-30T02:58:40Z

WordPress/Classes/ViewRelated/Comments/FullScreenCommentReplyViewController.swift

    // Static margin between the suggestions view and the text cursor position
    private let suggestionViewMargin: CGFloat = 5

+    var viewModel: FullScreenCommentReplyViewModelType = FullScreenCommentReplyViewModel()


[Nit] I don't think this variable should be public. How about injecting it through an initializer?

Good idea, I thought about injecting through an initializer at first, but as it was inheriting from an Objective-C class, I thought it would be more complex. After a deeper look, I realized it was easier than I thought.
Thanks for suggesting it.

hassaanelgarem · 2022-08-30T03:06:10Z

WordPress/WordPressTest/Comments/Controllers/FullScreenCommentReplyViewModelTests.swift

+        XCTAssertNotNil(tableView)
+    }
+
+    func testShouldShowSuggestionsIsFalse() {


This test is a bit vague. Can you either add a more verbose setup in the test or add some documentation?

Sure. I added a more verbose setup. Is it ok like that?

It's still unclear to me why the expected result is false 🤔

It runs on the test environment with the mocked CoreData context, so it is expected to return false. I will add some documentation and one more test to clarify.

…to inject its view model

hassaanelgarem · 2022-08-31T12:30:05Z

WordPress/WordPressTest/Comments/Controllers/FullScreenCommentReplyViewModelTests.swift

+        XCTAssertNotNil(tableView)
+    }
+
+    func testShouldShowSuggestionsIsFalse() {


It's still unclear to me why the expected result is false 🤔

hassaanelgarem · 2022-08-31T13:09:35Z

WordPress/Classes/ViewRelated/Comments/FullScreenCommentReplyViewController.swift

+        // nibName from super class
+        let nibName = String(describing: EditCommentViewController.self)
+        self.viewModel = viewModel
+        super.init(nibName: nibName, bundle: nil)


[Nit]
EditCommentViewController has a static function (+ (NSString *)nibName;) that determines the nibName that we can leverage. If we make this public we can do something like this:

Suggested change

// nibName from super class

let nibName = String(describing: EditCommentViewController.self)

self.viewModel = viewModel

super.init(nibName: nibName, bundle: nil)

self.viewModel = viewModel

super.init(nibName: Self.nibName(), bundle: nil)

What do you think?

Nice catch. I applied your suggestions. Thank you!

…classes

…mentReplyViewController

hassaanelgarem · 2022-09-04T22:52:27Z

LGTM!
One last thing, please add a low-priority release note for this fix 🙏

igoriols added 4 commits August 25, 2022 00:00

Add: create FullScreenCommentReplyViewModel and its tests

e624c8b

Change: update FullScreenCommentReplyViewController to use its view m…

2398b5a

…odel

Change: set last suggestions search text when opening FullScreenComme…

e5847ab

…ntReplyViewController and make suggestions visible if needed

Change: set last suggestions search text when closing FullScreenComme…

62229f4

…ntReplyViewController and make suggestions visible if needed

hassaanelgarem self-requested a review August 28, 2022 23:24

hassaanelgarem self-assigned this Aug 28, 2022

hassaanelgarem added [Type] Bug Mentions labels Aug 28, 2022

hassaanelgarem added this to the 20.7 milestone Aug 28, 2022

hassaanelgarem reviewed Aug 30, 2022

View reviewed changes

igoriols added 3 commits August 31, 2022 03:04

Fix: code formatting

e4e903b

Test: add a more verbose setup to testShouldShowSuggestionsIsFalse

a6bc358

Change: add new constructors to FullScreenCommentReplyViewController …

8dfb5eb

…to inject its view model

igoriols requested a review from hassaanelgarem August 31, 2022 09:29

hassaanelgarem reviewed Aug 31, 2022

View reviewed changes

igoriols added 2 commits August 31, 2022 17:19

Change: expose EditCommentViewController.nibName to be used in child …

035a977

…classes

Test: update test in FullScreenCommentReplyViewModelTests

629a0a4

igoriols requested a review from hassaanelgarem August 31, 2022 20:24

igoriols added 4 commits September 3, 2022 20:08

Test: save the blog in the mocked database when testing FullScreenCom…

bdc4ec4

…mentReplyViewController

Test: update test documentation in FullScreenCommentReplyViewModelTests

0936107

Test: format FullScreenCommentReplyViewModelTests

81ec14d

Test: save the blog in the mocked database when testing FullScreenCom…

4e4bd01

…mentReplyViewController

hassaanelgarem approved these changes Sep 4, 2022

View reviewed changes

hassaanelgarem modified the milestones: 20.7, 20.8 Sep 4, 2022

igoriols added 2 commits September 5, 2022 12:14

Merge branch 'trunk' into feature/19051-keep-suggestions-list-visible

9d7d7ab

Add: release note

9eca285

hassaanelgarem mentioned this pull request Sep 5, 2022

Temporary PR to run CI on forked branch #19287

Merged

Merge branch 'trunk' into feature/19051-keep-suggestions-list-visible

3ff8801

hassaanelgarem enabled auto-merge September 6, 2022 00:48

hassaanelgarem merged commit b68cca3 into wordpress-mobile:trunk Sep 6, 2022

ravishanker mentioned this pull request Sep 21, 2022

Suggestions List (using @mention) sometimes lags or appears blank #19329

Closed

Conversation

igoriols commented Aug 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Details

Preview

Testing Instructions

Regression Notes

Uh oh!

hassaanelgarem commented Aug 29, 2022

Uh oh!

igoriols commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hassaanelgarem commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igoriols commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hassaanelgarem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hassaanelgarem Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 30, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

igoriols Aug 31, 2022

Choose a reason for hiding this comment

Uh oh!

hassaanelgarem commented Sep 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

igoriols commented Aug 26, 2022 •

edited

Loading

igoriols commented Aug 29, 2022 •

edited

Loading

hassaanelgarem commented Aug 29, 2022 •

edited

Loading

igoriols commented Aug 29, 2022 •

edited

Loading

igoriols Aug 31, 2022 •

edited

Loading

hassaanelgarem Aug 31, 2022 •

edited

Loading

igoriols Aug 31, 2022 •

edited

Loading

hassaanelgarem Aug 31, 2022 •

edited

Loading