886754 - [Contacts] No Search Highlight Apparent in Contact Search

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Description

[Eric Pang [:epang] UX]

Attachment: Highlight Search - Contacts

Pavel,

Can you please add search highlights for contact search?
This should function similarly to Email search.

The highlight should be:
#b2f2ff 100% Opacity

Thanks!

Comment 1

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Eric,
I think I'll do it slowly. Maybe Sam can do this faster than me.

Comment 2

[Eric Pang [:epang] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #1)
> Hey Eric,
> I think I'll do it slowly. Maybe Sam can do this faster than me.

Hi Sam,
Can you help us add the search highlight for contact search?

Comment 3

[Mark Shiao]

Hi guys,

I noticed this missing feature and added it while working on bug 866902.  The pull request is at https://github.com/mozshiao9/gaia/commit/e01b9a4b081ff7aa4b89df56ed83c0c282d852c3

The code hasn't been reviewed and is probably outdated since UX has been quite busy and unavailable to validate the visual/transition changes.  

Since bug 866902 is a WIP, I would like to know if I can take this and work on it. Given no major changes, my patch might still be applicable.

Thanks,
Mark

Comment 4

[Eric Pang [:epang] UX]

Pavel/Sam, where did we leave off on this?  Will the patch that Mark create help?

Comment 5

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I think Sam can tell more than me here

Comment 6

[Eric Pang [:epang] UX]

Hey Sam, can you help implement this?

Comment 7

[Eric Pang [:epang] UX]

Hi Pavel, I'm passing this bug back to you.  Can you help implement?  Let me know when it's ready and I can test and provide feedback.  Also when ready for review please flag to the component owner. Thanks!

Comment 8

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Hey Francisco,
can you check this one. I used the code from Mark Shiao

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 815366 [details]
patch for Gaia/master

Hi Pivanov,

can you review the unit tests:

https://travis-ci.org/mozilla-b2g/gaia/jobs/12373265

I've tried to pass them twice and getting that error.

Thanks a lot for your work!

Comment 11

[Eric Pang [:epang] UX]

(In reply to Francisco Jordano [:arcturus] from comment #10)
> Comment on attachment 815366 [details]
> patch for Gaia/master
> 
> Hi Pivanov,
> 
> can you review the unit tests:
> 
> https://travis-ci.org/mozilla-b2g/gaia/jobs/12373265
> 
> I've tried to pass them twice and getting that error.
> 
> Thanks a lot for your work!

Hey Pavel, can you take a look at Francisco's Comment?  Thanks!

Comment 12

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Eric,
I fixed some of the issues but still have one and I write email to Francisco for suggestion and now waiting for answer

Comment 13

[Eric Pang [:epang] UX]

(In reply to Pavel Ivanov [:ivanovpavel] from comment #12)
> Hey Eric,
> I fixed some of the issues but still have one and I write email to Francisco
> for suggestion and now waiting for answer

ok sounds good! Thanks Pavel!

Comment 14

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Note: I need a device for this one

Comment 15

[David Scravaglieri [:scravag]]

Not a blocker please land it once it is ready.

Comment 16

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Hey Sam,

Did you have a chance to look at this one?

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

ni? Sam
Hi Sam, i wonder if this bug is still worked on. Thanks

Comment 18

[Michał Budzyński (:michalbe)]

I'll be happy to take it.

Comment 19

[Michał Budzyński (:michalbe)]

I've work on this bug in branch 'Bug886754' in my repo [1]. In the first WIP patch I catch all the nodes that we are interested in (when searching will go thru all the contacts) and passing them to the 'highlightNode' function [2]. Working on this function now.

[1] https://github.com/michalbe/gaia/tree/Bug886754
[2] https://github.com/michalbe/gaia/commit/6a87d28e7fdb4e63db01ac23898c2948af612ca1

Comment 20

[Michał Budzyński (:michalbe)]

I've created a Regular Expression that match searched phrase in the new WIP patch [1]. 

[1] https://github.com/michalbe/gaia/commit/0c3aef4a5d7594e599fdbfee8b22daa0c1511f1b#diff-d965a5a85955e09a269c4a64142c0956R402

Comment 21

[Michał Budzyński (:michalbe)]

Previous patch worked only on first letter because when regex was trying to match the second one, the previous was already wrapped in highlighting spans. This commit solves this problem [1].

[1] https://github.com/michalbe/gaia/commit/9b335ca40931e69f1a800a9f9c22054147e1fc63

Comment 22

[Michał Budzyński (:michalbe)]

Because of the noticeable delay of highlighting on a huge contacts list, the next patch [1] operates on single nodes while adding them to the list, not on a whole list. The delay doesn't occur anymore.

[1] https://github.com/michalbe/gaia/commit/055a6afa8968bc19441c6b3ff025249ec87eb397

Comment 23

[Michał Budzyński (:michalbe)]

Next commit [1] fixes multiple letters highlight.

[1] https://github.com/michalbe/gaia/commit/927e7083b99127ff57d2f573a76b361080ec1520

Comment 24

[Michał Budzyński (:michalbe)]

Because on the initial search only one chunk of contacts is displayed, highlighting needs to be executed also when we display remaining results [1].

[1] https://github.com/michalbe/gaia/commit/e075c84edad096d2f0b06fda3edf659754936bc3

Comment 25

[Michał Budzyński (:michalbe)]

Final patch is ready[1], working on tests now.

[1] https://github.com/michalbe/gaia/commit/3f4491ce57ebdd23a53c5cb48420ef259143f9e9

Comment 26

[Michał Budzyński (:michalbe)]

Attachment: Final Patch

Comment 27

[Michał Budzyński (:michalbe)]

Final patch ready with test. Waiting for r? from :bkelly

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Sorry, I was out a bit today.  I will most likely review this tomorrow morning (UTC-4).

Comment 29

[Michał Budzyński (:michalbe)]

Great, thanks.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8414418 [details] [review]
Final Patch

Looks good!  I just requested a few explanatory comments on github.  I verified that it works well on my buri with reference-workload-heavy.  This also adds tests and travis is green.

Well done! r=me

Comment 31

[Michał Budzyński (:michalbe)]

Thank you for your r Ben! I've updated the comments, Travis is green, patch landed in master [1]. 

[1] https://github.com/mozilla-b2g/gaia/commit/8f40d7e84b3070807259329db0112e812b00d681

Comment 32

[Francisco Jordano [:arcturus] [:francisco]]

\o/