920445 - [Flatfish][homescreen] fix search_page enabled customization

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[[e-mail]]

current search_page enabled customization are broken due to new e.me integration to start screen

expect:

can disable search bar via customization

Comment 1

[[e-mail]]

not shown e.me since its broken on tablet

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: Pointer to Github pull request 12572.html

Hi Vivien, 

In tablet build, we need to hide the search bar in ev.me page, so I updated the code base to fulfill this.

Hope you can take some time and review my code. Thanks ! :D

Comment 3

[Cristian Rodriguez (:crdlc)]

Hi All,

   Actually I don't know how it can work... What happens with collections for example?

Thanks

Comment 4

[Cristian Rodriguez (:crdlc)]

Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html

Hi,


    To achieve this you have to take in account:

1) Make sure that collections are removed from grid in applications-data.js and empty pages after removing collections if it is needed

https://github.com/mozilla-b2g/gaia/blob/master/build/applications-data.js#L224

if (!search_page_enabled) {
   // delete collections and empty pages from customize.homescreens
}

2) Set a new class to body to indicate that search page is available

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/configurator.js#L29

document.body.classList.add('searchPageEnabled')

3) Go to grid.js

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid.js#L47

var MAX_ICONS_PER_EVME_PAGE = document.body.classList.contains('searchPageEnabled') ? 4 * 3 : 4 * 4 ;

4) Go to grid.css and add this class to:

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L59

.apps div:first-child ol > li:nth-child(13) --> body.searchPageEnabled .apps div:first-child ol > li:nth-child(13)

and the same here

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L65
https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/style/grid.css#L74 (please change 21 per 17 there is a mistake)

5) Review the method called startHomescreenByDefault, 'div[role="search-page"]' doesn't make sense anymore and implement here your code that was fine. There are only two sections, evmeContainer and evmeOverlay in the DOM

https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/configurator.js#L153

Thanks

Comment 5

[Cristian Rodriguez (:crdlc)]

Ran, do you think that I forgot something?

Comment 6

[Gregor Wagner [:gwagner]]

Adding Tim for the blocking decision for flatfish.

Comment 7

[[e-mail]]

Hi Juwei,

Can you help clarify what's the proper UX here?

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This needs to block because e.me should not show up on tablet devices.

Comment 9

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Cristian,

Thanks for your reply first. 

I am not sure why we have to remove collection when we disabled search_page ?

Comment 10

[Cristian Rodriguez (:crdlc)]

Collections are a feature of ev.me. If you disable evme, you are removing the new collection feature

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Cristian Rodriguez (:crdlc) from comment #10)
> Collections are a feature of ev.me. If you disable evme, you are removing
> the new collection feature

Thanks Chirsian. My current approach is to loop customize.homescreens and take off the one matches collections. Can you check my PR and give me some advices ? Thanks :P

Comment 12

[Cristian Rodriguez (:crdlc)]

Yeahh, in general it looks good, you have some suggestions on github. This is the good patch to reach this feature, great work you are doing

ps my name is Cristian jijiji

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html

Sorry about typing your name wrong, Cristian. Please forgive me !!!

I fixed some typos and bugs then run tests and it seems they all work well. 

Big thanks !

Comment 14

[Cristian Rodriguez (:crdlc)]

Don't worry, my teachers called me Cristina at school :)

Comment 15

[Steven Yang [:styang]]

It's not required for 1st launch of flatfish. koi-

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Cristian Rodriguez (:crdlc) from comment #14)
> Don't worry, my teachers called me Cristina at school :)

Haha Thanks Cristian !
 
By the way, I updated the code in Github and need your review ! Plz kindly review my PR again when you are free. Thanksss :D

Comment 17

[Cristian Rodriguez (:crdlc)]

Please Amir, review it as well because if this fails, it would be critical

Comment 18

[Amir Nissim (:amirn)]

Added comments on Github

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks for you guys' help. I also left some comments on Github.

Comment 20

[Cristian Rodriguez (:crdlc)]

Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html

Cool!

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Cristian Rodriguez (:crdlc) from comment #20)
> Comment on attachment 812572 [details]
> Pointer to Github pull request 12572.html
> 
> Cool!

Thanks Cristian, should I wait for Amir's review (?)

Comment 22

[Cristian Rodriguez (:crdlc)]

I think so, 10x

Comment 23

[Evyatar 'Tron' Amitay (everything.me)]

Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html

Looks good to me. added some minor optimization comments, but they're really not critical.

Comment 24

[Amir Nissim (:amirn)]

eragonj - is it possible for a user to switch from a "search_page enabled" version to a "search_page disabled" one?

In such a scenario any Collections installed on the device will turn unusable.

Comment 25

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Amir Nissim (Everything.me) from comment #24)
> eragonj - is it possible for a user to switch from a "search_page enabled"
> version to a "search_page disabled" one?
> 
> In such a scenario any Collections installed on the device will turn
> unusable.

Well, currently search_page is only enabled/disabled in build time, so it is impossible to do that now.

Besides, this patch is for the build time customization (especially tablet), if we want to support that feature in settings, maybe we can open another bug for it :D

Comment 26

[[e-mail]]

Amir, this patch is target for v1.2 minimum tablet support.
We'd not tend to let user disable the search bar since the bar should be added back once we fixed all search bar/e.me layout in tablet size device.

Comment 27

[Juwei Huang[:juwei]]

Checked with Fred, no need for UX support in this stage, thanks.

Comment 28

[Francis Lee [:frlee]]

according to triage result, it is koi+. e.me needs to be remove on flatfish.

Comment 29

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Landed on master : 3f9b4c8cf2bac6a2ffe07952ad875de82d1ecee9

Comment 30

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks all :]

Comment 31

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi @Amir, 

should we open a followup bug for this scenario to let users customize enable/disable e.me ?

If so, maybe we can discuss more about that.

Comment 32

[Amir Nissim (:amirn)]

I don't think so. I was only asking to make sure nothing breaks.
I will ask Ran and update if needed.

Thanks.

Comment 33

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Ok Thanks Amir !

Comment 34

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.2.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.2
  git cherry-pick -x -m1 3f9b4c8cf2bac6a2ffe07952ad875de82d1ecee9
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 35

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi John,

after quick check, this patch depends on https://bugzilla.mozilla.org/show_bug.cgi?id=910316. 

But it seems a huge patch, any idea (?)

Comment 36

[Jason Smith [:jsmith]]

(In reply to EJ Chen [:eragonj] from comment #35)
> Hi John,
> 
> after quick check, this patch depends on
> https://bugzilla.mozilla.org/show_bug.cgi?id=910316. 
> 
> But it seems a huge patch, any idea (?)

You need a branch-specific patch - bug 910316 is only being implemented on 1.3 or later.

Comment 37

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Amir,

Jason pointed that the dependency bug - https://bugzilla.mozilla.org/show_bug.cgi?id=910316 will not be landed in 1.2. I will open a new bug marked as koi? to fulfill this feature. 

I will ask you more details about the implementations in that bug. Thanks ;)

Comment 38

[Jason Smith [:jsmith]]

Moving this to 1.3+ since the implementation here can only land on 1.3. bug 930858 is targeting the flatfish fix for 1.2.