Patryk Adamczyk [:patryk] UX Reporter
	Description • 11 years ago

Update the look of the lockscreen to a new design.

	Amy Lee [:amylee] UX
	Comment 1 • 10 years ago

Lockscreen mockups

	Amy Lee [:amylee] UX
	Comment 2 • 10 years ago

Patch for review:

https://github.com/caseyyee/gaia/tree/1.4-lockscreen-update

	Amy Lee [:amylee] UX
	Comment 3 • 10 years ago

(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.

	Casey Yee
	Comment 4 • 10 years ago

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.

	Casey Yee
	Comment 5 • 10 years ago

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)

	Patryk Adamczyk [:patryk] UX Reporter
	Comment 6 • 10 years ago

See comment 6

	Amy Lee [:amylee] UX
	Comment 7 • 10 years ago

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.

	Amy Lee [:amylee] UX
	Comment 8 • 10 years ago

Unlock toggle spec

	Amy Lee [:amylee] UX
	Comment 9 • 10 years ago

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.

	Amy Lee [:amylee] UX
	Comment 10 • 10 years ago

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

	Amy Lee [:amylee] UX
	Comment 11 • 10 years ago

Mock to show lockscreen flow

	jachen
	Comment 12 • 10 years ago

Amy- Please do a brain dump to Fang so she can work with the developers in that time zone. Thanks!

	Amy Lee [:amylee] UX
	Comment 13 • 10 years ago

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.

	Amy Lee [:amylee] UX
	Comment 14 • 10 years ago

Just an FYI, there may be some minor visual updates to the lockscreen icons and toggle. I will post updated graphics here.

	Fang Shih [:fang]
	Comment 15 • 10 years ago

Got it! I'll check the spec before the meeting. See you at the meeting! Thanks!

	Amy Lee [:amylee] UX
	Comment 16 • 10 years ago

Updated Lockscreen flow

	Amy Lee [:amylee] UX
	Comment 17 • 10 years ago

Updated Lockscreen Spec

	Prateek Jadhwani [:prateekjadhwani]
	Comment 18 • 10 years ago

I dont see any UI updates for the user story 942837 : https://bugzilla.mozilla.org/show_bug.cgi?id=942837

	Stephany Wilkes
	Comment 19 • 10 years ago

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.

	howie [:howie]
	Comment 20 • 10 years ago

This ticket is being tracked for 2.0

	Amy Lee [:amylee] UX
	Comment 21 • 10 years ago

Updated lockscreen flow spec

	Amy Lee [:amylee] UX
	Comment 22 • 10 years ago

Updated lock screen spec with new unlock toggle controls

	Amy Lee [:amylee] UX
	Comment 24 • 10 years ago

SVG icons for lockscreen

	howie [:howie]
	Comment 25 • 10 years ago

feature, no blocking

	Stephany Wilkes
	Comment 26 • 10 years ago

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!

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 27 • 10 years ago

Yeah that's OK.

	Naoki Hirata :nhirata (please use needinfo instead of cc)
	Comment 28 • 10 years ago

https://moztrap.mozilla.org/manage/case/13061/
https://moztrap.mozilla.org/manage/case/13060/
https://moztrap.mozilla.org/manage/case/13059/
https://moztrap.mozilla.org/manage/case/13039/

Test cases may need to be modified to what gets implemented after UX/FC.

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 29 • 10 years ago

Hi Amy,

Could we have png files for the assets? As usual, 1x, 1.5x, and 2x versions are needed. Thanks.

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 30 • 10 years ago

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

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 31 • 10 years ago

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!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 32 • 10 years ago

Okay, those new fonts are on PR of bug 987872, not merged yet...

	Amy Lee [:amylee] UX
	Comment 33 • 10 years ago

(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

	Stephany Wilkes
	Comment 34 • 10 years ago

Thanks, Amy. Fang, please put reviews in this bug as comments. Thanks, all!

	Amy Lee [:amylee] UX
	Comment 35 • 10 years ago

Updated spec with camera icon in pin code screen

	Amy Lee [:amylee] UX
	Comment 36 • 10 years ago

Updated spec. Added camera icon to pin code screen.

	Amy Lee [:amylee] UX
	Comment 37 • 10 years ago

Updated spec. Added camera icon to pin code screen.

	Amy Lee [:amylee] UX
	Comment 38 • 10 years ago

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!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 39 • 10 years ago

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.

	Fang Shih [:fang]
	Comment 40 • 10 years ago

Hi John,
I've attached the png icons in @1, @1.5, and @2 scale. Thanks!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 41 • 10 years ago

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!

	Fang Shih [:fang]
	Comment 42 • 10 years ago

Hi John,
Camera icon attached. Thanks!

	Amy Lee [:amylee] UX
	Comment 43 • 10 years ago

My mistake. Here are the correct sizes for the music player icons in lockscreen. 

Thanks!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 44 • 10 years ago

WIP patch for almost everything excluding the "more notifications" vertical arrow as in "3+ Notifications"

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 45 • 10 years ago

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'.

	Stephany Wilkes
	Comment 46 • 10 years ago

Nice work, everyone. Can't wait to see this land!

	John Lu [:mnjul][Please NI or I'll miss]
	Comment 47 • 10 years ago

Master: https://github.com/mozilla-b2g/gaia/commit/7a09fc92d54368e7dcbe7ec76bf2ebf0ff854d8c

	Amy Lee [:amylee] UX
	Comment 48 • 10 years ago

Hi, 

I just wanted to check in if visual design has been flagged for UI review?

	Germán Toro del Valle (:gtorodelvalle)
	Comment 49 • 10 years ago

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!

	Fang Shih [:fang]
	Comment 50 • 10 years ago

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!

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 51 • 10 years ago

(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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 52 • 10 years ago

Hum, it looks like this bug should depend on that instead of blocks.

	Doug Sherk (:drs) (inactive)
	Comment 53 • 10 years ago

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.

	Greg Weng [:snowmantw][:gweng][:λ] Assignee
	Comment 54 • 10 years ago

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

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 55 • 10 years ago

Commented in bug 1041879 comment 8.

	Doug Sherk (:drs) (inactive)
	Comment 56 • 10 years ago

Let's keep the discussion for this in bug 1041879. I just posted bug 1041879 comment 10.