Page MenuHomePhabricator

Deploy flicker-free search for all users
Closed, ResolvedPublic3 Story Points

Description

The problem
Mobile web search results flicker while updating and typing in the entire query.

Proposed Interaction
Assumption - User has typed some query and some results are showing below the searchbar
Next steps- User types in more characters > we request for results > we don't hide the current results > If new results take more than 2s on client side we show the spinner as in the mocks > ELSE we update the results without any spinner

the results can be tapped on in the 2s gap.

Acceptance criteria

  • move it out of beta so everyone can enjoy this

Test plan

  • When carrying out searches search results should not disappear on typing. They should be replaced when new results are available
  • A spinner should only show when the search is taking longer than 2s. You can simulate this by disabling your internet connection before carrying out a search.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 22 2016, 8:38 PM

@bmansurov we have a different behavior on everyone else?? where do we show the spinner for regular users?

@bmansurov wait.. the recent changes we did to remove the flickr from auto suggest weren't deployed for everyone?

Nope, beta only.

@bmansurov hmm i thought we deployed it to everyone. can we change this task to deploy it for everyone instead of feature flagging? cc @ovasileva can confirm after the break.

Sounds good. Feel free to update the description as you see fit.

Nirzar renamed this task from Feature flag the search overlay delay functionality to Deploy flicker-free search for all users.Dec 22 2016, 10:41 PM
Nirzar updated the task description. (Show Details)

@bmansurov description updated.

bmansurov removed bmansurov as the assignee of this task.Jan 3 2017, 4:39 PM

Thanks.

Any reason this is still in beta? Any concerns about moving this to stable that would make it more than a 2 pointer?

Nirzar added a comment.Mar 7 2017, 7:43 PM

why is this in beta? i thought we shipped it

Any technical blockers @bmansurov ...?

Any technical blockers @bmansurov ...?

None that I know of.

As I side note, from the original task:

This is a minor change to current behavior and we would like to see how it improves the experience. if we see value in it we can invest more time.

@Nirzar, did we see value in the new experience? If yes, how did we measure?

Nirzar added a comment.EditedMar 8 2017, 9:36 PM

@Nirzar, did we see value in the new experience? If yes, how did we measure?

as this is a minor change. and not a feature we wouldn't be spending resourcing in evaluating the user value formally. this change is purely based on design principles, best practices, and judgement from design.

@Nirzar, your comment brings up a good point: Why was this feature not built into stable from ground up, do you remember? This is a good time to come to an agreement on when to enable features in beta and in stable. It will help us developers craft a solution depending on the expectations from the designer and PO.

Nirzar added a comment.Mar 8 2017, 9:53 PM

Good point. Maybe we can have some norms around that. There are obviously things that need to go to stable directly but reason we said "feature flag everything" is because we are thinking of any features of bigger UI changes. now those two terms have ambiguity in them so we need norms around what to call a change. maybe create buckets and have different deployment strategy for each buckets.

Let me create a doc and share with the team.

@Nirzar, your comment brings up a good point: Why was this feature not built into stable from ground up, do you remember? This is a good time to come to an agreement on when to enable features in beta and in stable. It will help us developers craft a solution depending on the expectations from the designer and PO.

The code was introduced by @bmansurov in rEMFR4c64c8c8d6bd: Beta: Improve `search` experience as part of T137068 which Sam edited back in June 2016 to say MFBeta only. We can only assume there was a reason for adding this beta requirement. I'm not sure it's necessary to work out why now, it was probably meant as a short term test and we did a bad job at seeing it through to completion. Instead let's focus on writing clearer tasks upfront which document our decisions and reasoning.

Good point. Maybe we can have some norms around that. There are obviously things that need to go to stable directly but reason we said "feature flag everything" is because we are thinking of any features of bigger UI changes. now those two terms have ambiguity in them so we need norms around what to call a change. maybe create buckets and have different deployment strategy for each buckets.

We do have norms. We agreed to feature flag everything and we now add acceptance criteria to create tasks for follow up work as part of sign off, something we didn't have back in June 2016.

Are those norms cover the justification this card to be in beta and then stable? Baha's question will also get answered with that.

My point is there probably was justification for putting this in beta but it was not recorded so lost in history. I am hopeful now the team has improved since then in how it approaches work this will not happen again.

With respect to this task this looks good for estimation and implementation.

I'll tag it for next sprint, I think we can make space

Change 347636 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Deploy flicker free search

https://gerrit.wikimedia.org/r/347636

ovasileva set the point value for this task to 3.Apr 11 2017, 4:43 PM

@Nirzar - do we want to show the loading animation still?

Is this how the spinner should show?

Just checking as it's not clear from the description and that's how it's implemented...

New patch is up. I also did a follow up as I don't like the fact the SearchOverlay is using global events.

@Jdlrobson can we center align the spinner?

the mock is here > https://phabricator.wikimedia.org/T137068

in the mock, there's no keyboard but expected behavior is spinner center aligns with the viewport

phuedx added a subscriber: phuedx.Apr 20 2017, 5:51 PM

rEMFR8721d8a9e4d9: Deploy flicker free search works as expected but the spinner isn't center aligned yet so I've just +1'd it.

Horizontal alignment has been taken care of.

Change 347636 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove beta specific search behavioural changes

https://gerrit.wikimedia.org/r/347636

Jdlrobson updated the task description. (Show Details)Apr 20 2017, 11:37 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson added a subscriber: ABorbaWMF.

@ABorbaWMF with regards to QA, I hope the above test plan makes sense?

Jdlrobson reassigned this task from Jdlrobson to ABorbaWMF.Apr 20 2017, 11:37 PM
phuedx added a comment.EditedApr 21 2017, 8:35 AM

A spinner should only show when the search is taking longer than 2s. You can simulate this by disabling your internet connection before carrying out a search.

@ABorbaWMF: The developer tools of a lot of modern browsers also let you simulate arbitrary delays in the network connection. BrowserStack definitely has these tools enabled.

phuedx removed ABorbaWMF as the assignee of this task.Apr 26 2017, 9:56 AM
Jdlrobson assigned this task to Nirzar.Apr 26 2017, 4:46 PM

Please can you sign off!