Closed
Bug 920445
Opened 11 years ago
Closed 11 years ago
[Flatfish][homescreen] fix search_page enabled customization
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect)
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
Reporter | ||
Comment 1•11 years ago
|
||
not shown e.me since its broken on tablet
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → ejchen
Reporter | ||
Updated•11 years ago
|
Summary: [homescreen] fix search_page enabled customization → [Flatfish][homescreen] fix search_page enabled customization
Assignee | ||
Comment 2•11 years ago
|
||
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)
Attachment #812572 -
Flags: review?(21) → review?(crdlc)
Comment 3•11 years ago
|
||
Hi All, Actually I don't know how it can work... What happens with collections for example? Thanks
Comment 4•11 years ago
|
||
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-
Comment 6•11 years ago
|
||
Adding Tim for the blocking decision for flatfish.
Flags: needinfo?(timdream)
Reporter | ||
Comment 7•11 years ago
|
||
Hi Juwei, Can you help clarify what's the proper UX here?
Flags: needinfo?(jhuang)
Comment 8•11 years ago
|
||
This needs to block because e.me should not show up on tablet devices.
blocking-b2g: koi? → koi+
Flags: needinfo?(timdream)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
Collections are a feature of ev.me. If you disable evme, you are removing the new collection feature
Flags: needinfo?(crdlc)
Assignee | ||
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
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
Assignee | ||
Comment 13•11 years ago
|
||
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)
Comment 14•11 years ago
|
||
Don't worry, my teachers called me Cristina at school :)
Updated•11 years ago
|
QA Contact: atsai
Comment 15•11 years ago
|
||
It's not required for 1st launch of flatfish. koi-
blocking-b2g: koi+ → ---
Assignee | ||
Comment 16•11 years ago
|
||
(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)
Updated•11 years ago
|
Attachment #812572 -
Flags: review?(amirn)
Comment 17•11 years ago
|
||
Please Amir, review it as well because if this fails, it would be critical
Flags: needinfo?(crdlc)
Comment 18•11 years ago
|
||
Added comments on Github
Assignee | ||
Comment 19•11 years ago
|
||
Thanks for you guys' help. I also left some comments on Github.
Comment 20•11 years ago
|
||
Comment on attachment 812572 [details]
Pointer to Github pull request 12572.html
Cool!
Attachment #812572 -
Flags: review?(crdlc) → review+
Assignee | ||
Comment 21•11 years ago
|
||
(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•11 years ago
|
||
I think so, 10x
Updated•11 years ago
|
Attachment #812572 -
Flags: review?(evyatar)
Comment 23•11 years ago
|
||
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+
Comment 24•11 years ago
|
||
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)
Assignee | ||
Comment 25•11 years ago
|
||
(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)
Reporter | ||
Comment 26•11 years ago
|
||
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•11 years ago
|
||
Checked with Fred, no need for UX support in this stage, thanks.
Flags: needinfo?(jhuang)
Comment 28•11 years ago
|
||
according to triage result, it is koi+. e.me needs to be remove on flatfish.
blocking-b2g: --- → koi+
Updated•11 years ago
|
Attachment #812572 -
Flags: review?(amirn) → review+
Assignee | ||
Comment 29•11 years ago
|
||
Landed on master : 3f9b4c8cf2bac6a2ffe07952ad875de82d1ecee9
Assignee | ||
Comment 30•11 years ago
|
||
Thanks all :]
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 31•11 years ago
|
||
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)
Comment 32•11 years ago
|
||
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)
Assignee | ||
Comment 33•11 years ago
|
||
Ok Thanks Amir !
Comment 34•11 years ago
|
||
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)
Assignee | ||
Comment 35•11 years ago
|
||
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)
Comment 36•11 years ago
|
||
(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.
Assignee | ||
Comment 37•11 years ago
|
||
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•11 years ago
|
||
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+
Updated•11 years ago
|
Flags: needinfo?(ran)
You need to log in
before you can comment on or make changes to this bug.
Description
•