Closed Bug 920445 Opened 7 years ago Closed 6 years ago

[Flatfish][homescreen] fix search_page enabled customization

Categories

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

Other
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+)

RESOLVED FIXED
blocking-b2g 1.3+

People

(Reporter: gasolin, Assigned: eragonj)

References

Details

Attachments

(1 file)

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

expect:

can disable search bar via customization
not shown e.me since its broken on tablet
Blocks: flatfish
blocking-b2g: --- → koi?
Depends on: 869363
Assignee: nobody → ejchen
Summary: [homescreen] fix search_page enabled customization → [Flatfish][homescreen] fix search_page enabled customization
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
Attachment #812572 - Flags: review?(21)
Hi All,

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

Thanks
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
Attachment #812572 - Flags: review?(crdlc) → review-
Ran, do you think that I forgot something?
Flags: needinfo?(ran)
Adding Tim for the blocking decision for flatfish.
Flags: needinfo?(timdream)
Hi Juwei,

Can you help clarify what's the proper UX here?
Flags: needinfo?(jhuang)
This needs to block because e.me should not show up on tablet devices.
blocking-b2g: koi? → koi+
Flags: needinfo?(timdream)
Hi Cristian,

Thanks for your reply first. 

I am not sure why we have to remove collection when we disabled search_page ?
Flags: needinfo?(crdlc)
Collections are a feature of ev.me. If you disable evme, you are removing the new collection feature
Flags: needinfo?(crdlc)
(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
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 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 !
Attachment #812572 - Flags: review- → review?(crdlc)
Don't worry, my teachers called me Cristina at school :)
QA Contact: atsai
It's not required for 1st launch of flatfish. koi-
blocking-b2g: koi+ → ---
(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
Flags: needinfo?(crdlc)
Attachment #812572 - Flags: review?(amirn)
Please Amir, review it as well because if this fails, it would be critical
Flags: needinfo?(crdlc)
Added comments on Github
Thanks for you guys' help. I also left some comments on Github.
Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html

Cool!
Attachment #812572 - Flags: review?(crdlc) → review+
(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 (?)
I think so, 10x
Attachment #812572 - Flags: review?(evyatar)
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.
Attachment #812572 - Flags: review?(evyatar) → review+
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.
Flags: needinfo?(ejchen)
(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
Flags: needinfo?(ejchen)
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.
Checked with Fred, no need for UX support in this stage, thanks.
Flags: needinfo?(jhuang)
according to triage result, it is koi+. e.me needs to be remove on flatfish.
blocking-b2g: --- → koi+
Attachment #812572 - Flags: review?(amirn) → review+
Landed on master : 3f9b4c8cf2bac6a2ffe07952ad875de82d1ecee9
Thanks all :]
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(amirn)
I don't think so. I was only asking to make sure nothing breaks.
I will ask Ran and update if needed.

Thanks.
Flags: needinfo?(amirn)
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
Flags: needinfo?(ejchen)
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 (?)
Flags: needinfo?(ejchen)
Blocks: 927749
Depends on: 928169
Depends on: 910316
(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.
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 ;)
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.
blocking-b2g: koi+ → 1.3+
Blocks: 930858
Flags: needinfo?(ran)
You need to log in before you can comment on or make changes to this bug.