Closed Bug 1042105 Opened 5 years ago Closed 5 years ago
[User Story] Icon prioritization in the status bar
As a user, I want my icons to be prioritized in the status bar so that I always see the most important icons. Acceptance Criteria: 1. Status bar icons in the status bar are shown based on how many are able to fit between the collapsed Rocketbar and the right edge of the screen. 2. Status bar icons are displayed based on the priority specified in the UX spec. 3. The ordering of the status bar icons is as specified in the UX spec.
No description provided.
blocking-b2g: --- → backlog
feature-b2g: --- → 2.1
The latest version of the UX spec is located here: https://mozilla.box.com/s/tu5wlt6z385dh44kr5my - Rob
FYI, my WIP branch is here: https://github.com/gmarty/gaia/tree/Bug-1042105-Icon-prioritization-status-bar
Hey Michael, can you have a quick look at the work and see if the approach looks good to you. It's working but need optimisations (like caching icon width to avoid workflows each time) and some edge cases need to be addressed.
Attachment #8464807 - Flags: feedback?(mhenretty)
Comment on attachment 8464807 [details] Github wip branch I'm giving it a fb+, since I think the overall approach of measuring the width of space available and then showing/hiding icons by priority is sound. I do have some concerns about some of the implementation details, and I left those as comments on github. Let me know if you have any questions.
Attachment #8464807 - Flags: feedback?(mhenretty) → feedback+
Btw Guillaume, here is what the transition should look like when opening and closing the rocketbar. You don't need to do this in your patch obviously, but it is something to keep in mind.
Just as an update to anyone who is following along: we are going to go with a different approach suggested by Vivien where we have 2 versions of the statusbar, one minimized and one maximized. This will make it easier to achieve the transition shown in attachment 8465121 [details].
A wip of the new approach is in this branch: https://github.com/gmarty/gaia/tree/Bug-1042105-Icon-prioritization-Dual-Status-bar
Michael, can you please give me your feedback on the new approach? To try this patch, you must also apply the patch by Vivien from https://bugzilla.mozilla.org/show_bug.cgi?id=1047988 (no conflicts).
I tried to work on an implementation that doesn't clone the status bar on each icon refresh, as suggested by Vivien. But it turns out there is a lot of code duplication and it's harder to read if we want to maintain performance. I kept this work in a separate branch. Michael, can you have a quick look at that branch too? I'm pretty sure it's too messy to continue in this way, but wanted your thought on that. So maybe for the time being keep the previous code (that clones a DOM element regularly) and work on this optimisation later.
Comment on attachment 8467159 [details] Github branch Left some comments on github. Overall though, no matter what approach we take we will need something like this.
Attachment #8467159 - Flags: feedback?(mhenretty) → feedback+
Comment on attachment 8467881 [details] Github branch I see what you mean by messy. There are some things we can do to clean make this neater I think. What was Vivien's reason for wanting to not do cloneNode? Performance? Has this been measured?
Because I haven't noticed any performance issues on a 256Mb Flame and we want this to land ASAP, I think that's better to start with this DOM element cloning approach. We can open a followup bug to measure and fix potential performance issues. Alive, can you have a look at this PR?
Comment on attachment 8468446 [details] [review] Github PR I am not sure what is going on and I am surprised that there no tests for lots of new code and the reason is "we need to land it asap". I cannot review something like this without strict follow up plan. If the plan is totally rewrite because of spec change tremendously and it is worthy to, what's the reason to keep this? Land something quickly to unblock other's work sounds fairly nice, but as my experience people won't care the code landed anymore because the feature completed and just works. Sorry, please find another reviewer if you want to land it anyway. Vivien seems the one better than me.
Comment on attachment 8468446 [details] [review] Github PR I picked you because you had an empty review queue, but you are totally right, Vivien is a more appropriate reviewer here. I fixed the test suite so that it works with the code while I'm writing new ones to test the new features in a followup bug. Vivien, can you please review this PR?
Now we have proper unit tests for that feature, see the PR for details.
Comment on attachment 8468446 [details] [review] Github PR The code needs a bit more love but overall the approach should work. The only cases where I'm worried about cloning everything is when there is some network activity. When a web page loads it will trigger some network activity, and this icon will be added / removed multiple times. That may slow down page loading as it will results into relatively big changes into the DOM. We should probably land this code as if (with the comments addressed) and then open a followup to find a better way of updating the icons in order to optimize this use case and avoid to regress page load. Feel free to ask r? again once the comments are addressed. Also I discussed a bit with gmarty on IRC and some of the icons may need more adjustment in the minimized case. For example the battery icon could be vertical instead of horizontal in this case, which will save some spaces. Also the second SIM indicator does not dissapear yet if there is no SIM into it. That's probably a separate issue though, but it worth keeping tracks of those.
Comment on attachment 8468446 [details] [review] Github PR Vivien, I addressed your remarks in the latest version of this PR and left some comments in the obsolete one. Can you please review it again?
Comment on attachment 8468446 [details] [review] Github PR r+ with nits.
Attachment #8468446 - Flags: review?(21) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Pains me to do this as this is a super-important feature for 2.1, but we've had to back this out due to Gu test timeouts. It looks like the time between test runs takes longer. Note: we were also seeing this in the other statusbar patch that Mike was working on. The timeout is also apparent in the gaia-try that this originally landed with as well . Ni on Mike and Guillaume for notification.  https://github.com/mozilla-b2g/gaia/commit/e78e62125eb43c3a28cdc047987ba54430694a2f  https://tbpl.mozilla.org/?rev=620a053bac02aff3bcd22a66b475cd8ea31f1ead&tree=Gaia-Try
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As an user, I am really, really surprised that the time's priority is lower than some icons. https://bugzilla.mozilla.org/show_bug.cgi?id=1052333 Could we have a permanent time?
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #21) > As an user, I am really, really surprised that the time's priority is lower > than some icons. > https://bugzilla.mozilla.org/show_bug.cgi?id=1052333 > > Could we have a permanent time? Take a look at the spec above which defines the priorities: https://mozilla.box.com/s/tu5wlt6z385dh44kr5my What I'm seeing is that there seems to be enough room for the time, yet it's not being shown. (particularly when on the homescreen - the entire width should be used, per spec) I think there is still some tweaking going on, but we may need to make some modifications once we can play with it more.
Thanks Kevin for filing bug 1052241. This is now a huge priority since it blocks a bunch of work for 2.1.
Now that bug 1052241 has been fixed, let's see if the icon stuff passes gaia-try.
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Attaching spec so I can link to it.
You need to log in before you can comment on or make changes to this bug.