947097 - [Clock] Update tabs to use the new 1.3 designs

Status: RESOLVED FIXED

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

Description

[Eric Pang [:epang] UX]

Attachment: clock.png

Update clock app with new 1.3 tab designs

Comment 1

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Attachment: patch for Gaia/master

Comment 2

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

Handing off to Corey since I won't have time to get to this today.

Comment 3

[Corey Frang [:gnarf]]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

The display and behavior seem correct on device

This seems to be causing problems with the marionette test suite: https://travis-ci.org/mozilla-b2g/gaia/jobs/15027521 - I'm having a hard time figuring out where this test is going wrong.

Going to have to bounce this back to mike.

Comment 4

[Mike Pennisi [:jugglinmike]]

Corey: This bug is dependent on changes to the building blocks in bug 947093 . The Clock application's tab UI is non-functional when the attached patch is applied directly to `master`.

Pavel: Is that interpretation correct? If so, we may have to wait until bug 947093 is resolved before we're able to review this patch for correctness.

Comment 5

[Eric Pang [:epang] UX]

(In reply to Mike Pennisi [:jugglinmike] from comment #4)
> Corey: This bug is dependent on changes to the building blocks in bug 947093
> . The Clock application's tab UI is non-functional when the attached patch
> is applied directly to `master`.
> 
> Pavel: Is that interpretation correct? If so, we may have to wait until bug
> 947093 is resolved before we're able to review this patch for correctness.

Hey Mike, Pavel can correct me if I am wrong, but the plan is to land bug 947093 together with this bug (and all the other patches for each app). We can't land anything until we have r+ from all app owners since we don't want visual inconsistencies.  Thanks for looking at this so quickly!

Comment 6

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Yep, we will land all bugs who depends on Bug 947093 together.

Comment 7

[Mike Pennisi [:jugglinmike]]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

(In reply to Eric Pang [:epang] from comment #5)
> Hey Mike, Pavel can correct me if I am wrong, but the plan is to land bug
> 947093 together with this bug (and all the other patches for each app). We
> can't land anything until we have r+ from all app owners since we don't want
> visual inconsistencies.  Thanks for looking at this so quickly!

The criticism of bug 940520 seem valid for this plan; in other words, splitting the patch into multiple bugs doesn't actually mean anything if we require they all land in unison.

That said, I know that this is a high-priority feature for you guys, and I want to help out. I've applied the patch at attachment 8343598 [details] [review] (from bug 947093) and reviewed the changes to Clock described by this patch. I am setting the "r+" flag here under the provision that there are no further changes to the patch this one is dependent on. If that patch changes, please flag me again, and I will review again ASAP.

Comment 8

[Eric Pang [:epang] UX]

(In reply to Mike Pennisi [:jugglinmike] from comment #7)
> Comment on attachment 8343602 [details] [review]
> patch for Gaia/master
> 
> (In reply to Eric Pang [:epang] from comment #5)
> > Hey Mike, Pavel can correct me if I am wrong, but the plan is to land bug
> > 947093 together with this bug (and all the other patches for each app). We
> > can't land anything until we have r+ from all app owners since we don't want
> > visual inconsistencies.  Thanks for looking at this so quickly!
> 
> The criticism of bug 940520 seem valid for this plan; in other words,
> splitting the patch into multiple bugs doesn't actually mean anything if we
> require they all land in unison.
> 
> That said, I know that this is a high-priority feature for you guys, and I
> want to help out. I've applied the patch at attachment 8343598 [details] [review]
> (from bug 947093) and reviewed the changes to Clock described by this patch.
> I am setting the "r+" flag here under the provision that there are no
> further changes to the patch this one is dependent on. If that patch
> changes, please flag me again, and I will review again ASAP.

thanks for the really quick review Mike (we really appreciate it!), there shouldn't be any changes unless someone finds an issue with one of the patches.

Comment 9

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

Landed to master:
https://github.com/mozilla-b2g/gaia/commit/d50defd2ada4ddae7c5da2a92e338af33c10d5d9

Comment 10

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Reopening as this looks like this isn't in the 1.3 branch:
https://github.com/mozilla-b2g/gaia/blob/v1.3/apps/clock/index.html

Please close when it lands in v1.3

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Maybe I might be wrong in reopening it.  The reason why I reopened it was because this was targetted for v1.3 and it's not in that branch.  Not sure the best way to handle that situation...

Comment 12

[Preeti Raghunath(:Preeti)]

Please ask for approvals.

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

needs approval; please approve?

Comment 15

[Preeti Raghunath(:Preeti)]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

Approving flag per triage

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8343602 [details] [review]
patch for Gaia/master

We are no longer taking the tab change in 1.3, we are taking it in 1.4