Closed Bug 950884 Opened 6 years ago Closed 6 years ago

[VsD Refresh] Lockscreen Visual Refresh

Categories

(Firefox OS Graveyard :: Gaia::System::Lockscreen, defect)

x86
macOS
defect
Not set

Tracking

(feature-b2g:2.0, tracking-b2g:backlog)

RESOLVED FIXED
2.0 S3 (6june)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: padamczyk, Assigned: gweng)

References

Details

(Whiteboard: ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60, 2.0, ft:system-platform] [p=5])

Attachments

(8 files, 14 obsolete files)

4.39 KB, application/zip
Details
856.09 KB, image/png
Details
922.86 KB, image/png
Details
25.47 KB, application/zip
Details
5.55 KB, application/zip
Details
17.79 KB, application/zip
Details
46 bytes, text/x-github-pull-request
gweng
: review+
Details | Review
1.72 MB, application/pdf
Details
Update the look of the lockscreen to a new design.
Assignee: nobody → pla
No longer depends on: 950172
Attached file Lockscreen.zip (obsolete) —
Lockscreen mockups
Attached image 2014-03-17-16-05-01.png (obsolete) —
(In reply to Amy from comment #2)
> Patch for review:
> 
> https://github.com/caseyyee/gaia/tree/1.4-lockscreen-update

Hey Casey, 

I flashed the patch and it looks like the new version haven't been updated yet.
Flags: needinfo?(kyee)
I sent you the wrong branch.  I need to bring it up to date with the changes in master.   I will post back here once I have done that.
Flags: needinfo?(kyee)
Amy, here is the working branch for the bug:
https://github.com/caseyyee/gaia/tree/bug-950884

There are a few things that need to be done still:
- Icons need to be replaced using new set from Peter.
- The unlock handle should be white
- There needs to be a default color range set.  Palette is only created on wallpaper changes currently.
- Still some refinements to play control needed but the style pretty much patches what amy had designed.

Additional notes:
- There has been changes to the unlock mechanism code in the last month so i've just taken the changes straight from master.   So some styles have reverted because of this.  (Should be dark background track with no border)
See comment 6
Flags: needinfo?(amlee)
Assignee: pla → nobody
No longer blocks: 950751
Blocks: 950751
No longer blocks: 950751
Attached image Pincode_Screen_Spec.png (obsolete) —
Hi, 

Here are my answers:

-Color and transparency ratio on the unlock slide bar.

I've attached a spec for the unlock toggle

-Do we need any color change on the notification bar based on the background color? Or is it just transparent? (This will have impact how color picker will be implemented).

I'm not quite following what you mean. The notification background colour is determined by the colour picker that Casey created.


- Is PIN code key pad style the same as keyboard? Also, what's the transparent ratio on the pin number area?

I've attached a spec for the pin number area. The PIN code key pad should be the same style as the keypad when you create the PIN code in Settings. Please see Settings -> Screen lock --> Passcode Lock

I'm working on a complete spec for the lockscreen and will attach it as soon as it's done in the next day or so. If you need any information sooner let me know and I'll post it.
Flags: needinfo?(amlee)
Attached image Unlock_Toggle_Spec.jpg (obsolete) —
Unlock toggle spec
Just an update, there has been some additional redesigning of the lock screen happening this week so the specs will come end of the week/early next week.
Component: Gaia::Homescreen → Gaia::System::Lockscreen
Attached image Proposed_Lockscreen_Changes.png (obsolete) —
Hi, 

I've attached two mocks of the proposed changes to lockscreen. I would like to know that these changes are possible before creating full specs. Currently there is no one assigned to this bug. Is Ivan Tsay the developer on this? Let me know if there are any questions, I would be happy to walk the developer through these proposed changes. Thanks
Flags: needinfo?(jachen)
Attached image Lockscreen_Flow.png (obsolete) —
Mock to show lockscreen flow
Flags: needinfo?(mtsai)
Attachment #8377246 - Attachment is obsolete: true
Attachment #8392406 - Attachment is obsolete: true
Amy- Please do a brain dump to Fang so she can work with the developers in that time zone. Thanks!
Flags: needinfo?(jachen) → needinfo?(fshih)
Attached image Lockscreen_1.4_Spec.png (obsolete) —
Hi Fang, 

I've attached the full specs for lockscreen. I'll email you to figure out a meeting time to do a knowledge transfer.
Attachment #8400184 - Attachment is obsolete: true
Attachment #8400185 - Attachment is obsolete: true
Attachment #8401958 - Attachment is obsolete: true
Just an FYI, there may be some minor visual updates to the lockscreen icons and toggle. I will post updated graphics here.
Got it! I'll check the spec before the meeting. See you at the meeting! Thanks!
Flags: needinfo?(mtsai)
Flags: needinfo?(fshih)
Attached image Lockscreen_Flow_1.4.png (obsolete) —
Updated Lockscreen flow
Attachment #8401959 - Attachment is obsolete: true
Attached image Lockscreen_1.4_Spec.png (obsolete) —
Updated Lockscreen Spec
Attachment #8402952 - Attachment is obsolete: true
I dont see any UI updates for the user story 942837 : https://bugzilla.mozilla.org/show_bug.cgi?id=942837
Prateek, have you seen the lockscreen spec attached to this bug? If it does not suffice please let us know what you are looking for.
blocking-b2g: --- → 2.0?
Whiteboard: ux-tracking, visual design, visual-tracking, bokken → ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60,ft:system-platform]
This ticket is being tracked for 2.0
Whiteboard: ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60,ft:system-platform] → ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60, 2.0, ft:system-platform]
Attached image Lockscreen_Flow.png (obsolete) —
Updated lockscreen flow spec
Attachment #8403324 - Attachment is obsolete: true
Attached image Lockscreen_Spec_2.0.png (obsolete) —
Updated lock screen spec with new unlock toggle controls
Attachment #8403325 - Attachment is obsolete: true
Attached image Lockscreen_Flow_2.0.png (obsolete) —
Attachment #8417516 - Attachment is obsolete: true
Attached file Lockscreen_Assets.zip
SVG icons for lockscreen
feature, no blocking
blocking-b2g: 2.0? → backlog
Assigning to Greg Weng, with whom Fang has been working. Setting ni? for Tim to see if this is OK. Tim, please reassign as necessary and/or update the target milestone for the sprint in which this might fit. Thank you!
Assignee: nobody → gweng
Flags: needinfo?(timdream)
Yeah that's OK.
Flags: needinfo?(timdream)
Target Milestone: --- → 2.0 S3 (6june)
Flags: in-moztrap?(nhirata.bugzilla)
feature-b2g: --- → 2.0
Hi Amy,

Could we have png files for the assets? As usual, 1x, 1.5x, and 2x versions are needed. Thanks.
Flags: needinfo?(amlee)
Hi Amy

With current releases we have a "PM" or "AM" after time string, like "8:36 PM", but in the spec it is omitted. Do we really want to remove the "PM"/"AM"? Thanks
Hi Amy (and sorry for the triple mentioning...)

Where did you get your Fira Sans Ultra Light font? (in your spec, "Time Text" in the "3+ Notifications" image). It appears that we're currently not shipping the specific font weight, ref: https://github.com/mozilla-b2g/moztt/tree/master/FiraSans-2.001 . Thanks!
Okay, those new fonts are on PR of bug 987872, not merged yet...
Depends on: Fira
(In reply to John Lu [:mnjul] @MoCoTaipei from comment #29)
> Hi Amy,
> 
> Could we have png files for the assets? As usual, 1x, 1.5x, and 2x versions
> are needed. Thanks.

(In reply to John Lu [:mnjul] @MoCoTaipei from comment #30)
> Hi Amy
> 
> With current releases we have a "PM" or "AM" after time string, like "8:36
> PM", but in the spec it is omitted. Do we really want to remove the
> "PM"/"AM"? Thanks

Hi John, 

Yes we are removing AM/PM. It seems obvious if the time is AM or PM and other OS's don't include AM/PM either.

Also, Fang will be taking over lockscreen assets/reviewing implementation. Thanks
Flags: needinfo?(amlee) → needinfo?(fshih)
Thanks, Amy. Fang, please put reviews in this bug as comments. Thanks, all!
Attached image Lockscreen_Flow_2.0.png (obsolete) —
Updated spec with camera icon in pin code screen
Attachment #8417533 - Attachment is obsolete: true
Attached image Lockscreen_Flow_2.0.png
Updated spec. Added camera icon to pin code screen.
Attachment #8427033 - Attachment is obsolete: true
Attached image Lockscreen_2.0_Spec.png
Updated spec. Added camera icon to pin code screen.
Attachment #8417532 - Attachment is obsolete: true
Attached file music_player_icons.zip (obsolete) —
Hi, 

Just an FYI that the music player controls in lockscreen have been made slightly bigger so it's easier for users to navigate (see Bug 1011710, comment 8). I've attached the larger music player controls here in @1, @1.5, and @2 scale. Thanks!
Hi Amy: [re: removal of AM/PM] If we want to remove "AM/PM", do we need to show 24hr time? For example, when it is 1:30PM in the afternoon, do we want to show "13:30" instead of "1:30"? Thanks.
Flags: needinfo?(amlee)
Hi John,
I've attached the png icons in @1, @1.5, and @2 scale. Thanks!
Flags: needinfo?(fshih)
Hi Fang,

Thank for the png icons! However, I need one extra set of the camera icon: as the camera on the pincode screen is larger (26*20px) than the one on the lockscreen track (21*16px), I need the camera icon png files (1x, 1.5x, 2x) for the pincode screen. Thanks!
Flags: needinfo?(fshih)
Attached file Pin_Camera_icon.zip
Hi John,
Camera icon attached. Thanks!
Flags: needinfo?(fshih)
My mistake. Here are the correct sizes for the music player icons in lockscreen. 

Thanks!
Flags: needinfo?(amlee)
Attachment #8427156 - Attachment is obsolete: true
Attached file Patch
WIP patch for almost everything excluding the "more notifications" vertical arrow as in "3+ Notifications"
Attachment #8429923 - Flags: review?(gweng)
Attachment #8429923 - Attachment description: WIP Patch → Patch
Comment on attachment 8429923 [details] [review]
Patch

Great work. Please fix some nits, especially add some comments to help to explain the code better, especially about the 'new style'.
Attachment #8429923 - Flags: review?(gweng) → review+
Nice work, everyone. Can't wait to see this land!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Hi, 

I just wanted to check in if visual design has been flagged for UI review?
Flags: needinfo?(fshih)
Attachment #8429923 - Flags: ui-review?(fshih)
Hi John, I am currently using your code for the call screen to attend calls when the lock screen is activated and I found an element (#lockscreen-mute) in system/index.html which does not seem to be used. Or is there any scenario where this element is used? Just to know if I should used in the call screen case :) Thanks!
Flags: needinfo?(jlu)
Hi John,

I think it is really close now. According to our discussed earlier. Just a couple things that need to adjust.  I’ve also put them into one pdf. You can refer to the file attached.

Some notes: 

1) Missing gradient layer on the top of lock screen, when there is no notifications, it should show a light gradient layer on the top of the background image. 

2) Time font: It's not Fira Sans Ultra Light yet, but updates for this should happen when font landed into the device. 
  
3) The "Toggle arrows” is a bit fuzzy, looks like it stretch a little bit so it should be shorter than the gray toggle circle also.

4) Unlock background stroke is a bit heavy, Stroke Gradient Opacity is: Top 100%, Bottom 0%


5) On the Pin lock screen, the camera icon is not centered.


6) The clear bottom is incorrect size of the Pin lock keypad. 


7) Notification is missing arrow icon, and also scrolling up with “Time” is not showing. But according to your feedback, we are not able to have this function right now. 

Note that notification gradient may still a bit greenish, maybe we can adjust this a bit. Thanks!
Flags: needinfo?(fshih)
Attachment #8429923 - Flags: ui-review?(fshih) → ui-review-
(In reply to Fang Shih from comment #50)
> Created attachment 8433852 [details]
> 2.0_lockscreen_bug_spec.pdf
> 

Fan, thank you for the UI review. I've filed bug 1020115 to follow-up.
No longer blocks: 1020115
Hum, it looks like this bug should depend on that instead of blocks.
No longer blocks: 1020115
Depends on: 1020115
Flags: needinfo?(jlu)
No longer depends on: 1021479
Whiteboard: ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60, 2.0, ft:system-platform] → ux-tracking, visual design, visual-tracking, bokken [ucid:SystemPlatform60, 2.0, ft:system-platform] [p=5]
Depends on: 1020653
Depends on: 1021301
No longer blocks: 1023642
Depends on: 1023642
Depends on: 1029145
I'm not sure how this landed as there are several problems here that I can see that should have prevented this. Please correct me if I'm wrong, as there may have been offline discussion or I may be missing something, but:

1) Greg Weng is not a peer on any module according to the Firefox OS Module Owners page, let alone Gaia::System. [1] It doesn't appear that this review was redirected from a module peer or owner. I see in comment 26 that he was assigned to it, but wasn't given the review. Perhaps Tim's silence here was an implicit go-ahead.
2) This patch has no tests added or even modified. [2]
3) This is less important, and maybe I'm nitpicking. However, there are some fairly obvious problems that I spotted from just a quick skim, without knowing anything about the code, and which were not caught in review. [3]

[1] https://wiki.mozilla.org/Modules/FirefoxOS
[2] ctrl+f "_test" on https://github.com/mozilla-b2g/gaia/pull/19712/files
[3] This list does not include whitespace, indentation, and stylistic problems I found.
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-61c16a0a0190e95d51031835f62b30d7R1064 - using a rowX class instead of just wrapping these elements with a row div
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-3fb4b6ea1cebdea57ada1ac50c2c9c7aR914 - repeating a variable declaration
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-61c16a0a0190e95d51031835f62b30d7R1010 - use of this .blank class instead of override patterns
https://github.com/mozilla-b2g/gaia/pull/19712/files#diff-335953a0df10e3fb9613930bca786abfR534 - overly specific CSS selectors all over this code

As a consequence of these problems, my review of bug 1041879 is going to take much longer than it should because I can't read the tests to see expected functionality, or get clues as to what the changes there may be breaking. There's also the bigger problem that this code is completely untested so changes to it are much more liable to break it.

These things happen, and that's ok. But at this point, I think it's reasonable to expect everyone knows the Gaia policies, and it's the job of reviewers to enforce these. Perhaps the bigger problem is that I can't actually find these policies documented anywhere, and we should fix that.
Flags: needinfo?(timdream)
Flags: needinfo?(jlu)
Flags: needinfo?(gweng)
1. Good. If peer is the perquisite to review a patch, I'm glad to give the responsibility to anybody has the title, while there are some patches reviewed by me may now lost the legitimacy now, although they're mostly delegated by System owner & peer.

2. It passed all tests on Gaia-Try[1] include linter check. I don't mean Gaia-Try is enough to protect us from regressions, but the result shows either

a. at least the existing tests are not broken with the patch
b. need to add a whole bunch of tests to get our code base stronger, but because of the risk and what we have adopted usually, this should be another standalone bug and patch to to that (to add tests 'files', not only add some lines).

3. In my experiences, reviewers make differences decisions when they're in different point of views and this includes the personal opinions in even different times. For example, one of my reviewer asked nothing about one of patch and give r+ without any issue. But the next time (after rebasing, maybe the GitHub CC mail noticed him), he gave about 20+ opinions with the same codes. Since the 'useNewStyle' is a workaround we introduced, what I concerned is to remove it to make our codebase more clear. And I saw what you addressed on the GitHub pages, they're most not critical errors, while

a. indentation issue: as you said:

'I'd say indentation here is off, but there's no real standard, as long as it's either 2 spaces or lined up with the previous line.'

I've seen lot's of tiny variations of indentations in our codebase. If you want to unify them and clear all code we should fire another bug to do that, and send a mail to dev-gaia to communicate people whom wrote all of those code. But if I'm permitted to express my opinion that I think the 2-spaces sometime is not clear as what we've left in our codebase, although I know people would like to forcibly ask this to eliminate the possible arguments.

b. stale comments & dead images: yes, my fault to miss them. Thanks.

c. 'You should remove this file from xfail.list and fix the linting errors.' -- While the purpose of this patch is removing the workaround, and as far as I know there seems no forcible requirement to ask every patch to fix xfail.list at the same time (from the patches I've submitted and the principles I may only know a little; or do we?), I'd say this seems depends on reviewers, so I don't think this is a fail to ask nothing about that. 

4. John discussed this offline with me at our office. And on real device test his phone has no serious visual breaking, while reviewer should delegate real UI grant to UI review and that's the thing I suppose John would do, that's why I said I 'assume' the UI is okay. However, maybe the word is not correct and reviewer should do more things UX does to test the patch.

5. Some unclear comments and code I missed maybe because the code was mainly written by me (which make the comments are meaningful) , so there is a review blindness I think. And I should consider if someone else suddenly jump in to re-write the code.

[1]: https://tbpl.mozilla.org/?rev=70d40a0943d8c5efbb6f4233ac6666e5aac3bdaa&tree=Gaia-Try
Flags: needinfo?(gweng)
Commented in bug 1041879 comment 8.
Flags: needinfo?(timdream)
Let's keep the discussion for this in bug 1041879. I just posted bug 1041879 comment 10.
Flags: needinfo?(jlu)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.