805133 - [Calendar] Calendar month view jumps between 5 and 6 rows (weeks), looks odd

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect, P3)

Description

[Patryk Adamczyk [:patryk] UX]

The calendar rows should be the same height each month, which means 7 rows of weeks.

Comment 1

[James Lal [:lightsofapollo]]

This is not hard but keep in mind we also have 4 (feb), 5 and 6 week months.

Comment 2

[Patryk Adamczyk [:patryk] UX]

I might have made a typo, cause no month should hit 7 rows. 5 & 6 are the common ones.
Maintaining 6 rows constantly would be the ideal solution, the extra pixel space is nice, but having smoother transitions (layout not shifting) is more desired, it just makes the calendar feel more solid.

In the case of 4 rows. You'd have row 2-5 be the current month. While 1 & 6 are the previous and next.

Comment 3

[steve]

Spoke with Vicky the visual designer of the app and conferred with Patryk.
This is what needs to happen: 
The cell size will remain the same. i.e. Maintain the size of 5 rows currently (a much more pleasing square shape).
Remove the date stamp in the bottom section(orange).
When you have a month with 6 weeks an extra line (week) is added to the bottom of the month view box, see link:
 
https://www.dropbox.com/s/n5og0ikj6qapta1/Calendar_0002_1c%28Month%29-6weeks.png

Transitions refer to transition document

Comment 4

[Eric Pang [:epang] UX]

Hi Sam, do you think you can fix this bug?

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Not tracking, polish - if a low risk fix is available please renominate along with approval nomination.

Comment 6

[Sam Joch [:samjoch]]

Attachment: Current 6 lines vs. 5 lines months

can't see the bug here ? Please provides more infos.

Comment 7

[Eric Pang [:epang] UX]

Attachment: Calender 6 Rows

Hi Sam, from reading the comments -- when a month has 6 lines another row should be added instead of shrinking vertically (into the space of 5). Let me know if you have any questions. Thanks!

Comment 8

[Sam Joch [:samjoch]]

Eric, can you provide a screenshot with the 'day title' and events in the list. 
Because when i set the row the same height as for a 5 weeks month, 
the calendar takes a lot of space.

Comment 9

[Eric Pang [:epang] UX]

Attachment: Calendar 5 and 6 rows

Hi Sam,
Here's a mock up of 5 and 6 rows side by side.  

Victoria, before Sam works on this, can you confirm if this is how this should be implemented? Thanks!

Comment 10

[Victoria Gerchinhoren [:vicky]]

Yes, that's how it should work. Moreover the orange subtitle with the date should not be there, and days in the future should be white background, while days on the past are light grey, days not corresponding to this month are medium grey and "Today" slot should be dark grey, I assume this is being solved in another bug.

Comment 11

[Sam Joch [:samjoch]]

Attachment: 6 rows month

Comment 12

[Sam Joch [:samjoch]]

Attachment: Github PR

Comment 13

[Eric Pang [:epang] UX]

Comment on attachment 758027 [details]
6 rows month

Looks good, thanks Sam!

Comment 14

[Eric Pang [:epang] UX]

(In reply to Eric Pang [:epang] from comment #13)
> Comment on attachment 758027 [details]
> 6 rows month
> 
> Looks good, thanks Sam!

The 6 rows looks good.  But Visual design is really off with the calendar.  I'm going to open a separate bug to address the visual issues.

Comment 15

[James Lal [:lightsofapollo]]

Comment on attachment 758028 [details]
Github PR

Hey Sam,

This works great on our launch devices but something about the math that calculates the height of the month area is wacky on the higher resolution devices (you can see this on the GS Peak or by changing the resolution on FF nightly)

Can you reset flag for review again when this is resolved?

Comment 16

[Sam Joch [:samjoch]]

Comment on attachment 758028 [details]
Github PR

I have updated the PR. Thanks.

Comment 17

[James Lal [:lightsofapollo]]

Comment on attachment 758028 [details]
Github PR

awesome work!

Comment 18

[James Lal [:lightsofapollo]]

in master https://github.com/mozilla-b2g/gaia/commit/a7f7ba9b2700dfff4af102b325a1c9d71ffd6b28

Comment 19

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

looks this PR made travis failed. Sam could you check it?
https://travis-ci.org/mozilla-b2g/gaia/builds/7871377

Comment 20

[Sam Joch [:samjoch]]

Attachment: Followup PR (fix test)

Thanks Yuren.

Here is a followup: https://github.com/mozilla-b2g/gaia/pull/10288
James: Can you review it please ?

Comment 21

[James Lal [:lightsofapollo]]

Attachment: borked months day view

Going to back out this out for now

Comment 22

[James Lal [:lightsofapollo]]

reverted here: https://github.com/mozilla-b2g/gaia/commit/6a97fe462c380867e163ba9be588bc2439eec012.

Fairly sure this is a zindexing thing.

Comment 23

[Jason Smith [:jsmith]]

Reopening per backout in comment 22

Comment 24

[James Lal [:lightsofapollo]]

Comment on attachment 760315 [details]
Followup PR (fix test)

resetting flag- sorry should have found this in the first run forgot to open settings panel

Comment 25

[Eric Pang [:epang] UX]

Sam, can you look into what went wrong here? Thanks!

Comment 26

[Sam Joch [:samjoch]]

Attachment: New PR Github

Comment 27

[Sam Joch [:samjoch]]

Attachment: 6 rows month

Comment 28

[James Lal [:lightsofapollo]]

Comment on attachment 803783 [details]
New PR Github

One of you up for this?

Comment 29

[Kevin Grandon :kgrandon]

Comment on attachment 803783 [details]
New PR Github

I might provide some comments, but will let Gareth review this one.

Comment 30

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 803783 [details]
New PR Github

Thanks for the patch Sam. It's very close to ready I think. Flag me again once you get a chance to look at the changes I suggested and questions I asked!

Comment 31

[Eric Pang [:epang] UX]

(In reply to Gareth Aye [:gaye] from comment #30)
> Comment on attachment 803783 [details]
> New PR Github
> 
> Thanks for the patch Sam. It's very close to ready I think. Flag me again
> once you get a chance to look at the changes I suggested and questions I
> asked!

Hey Sam, can you look at the suggestions and questions Gareth asked? Thanks!

Comment 32

[Jason Smith [:jsmith]]

I don't see why this is nomed 1.3 - new calendar features isn't a priority for 1.3. This shouldn't block that release.

Comment 33

[Dylan Oliver [:doliver]]

triage: definitely not blocking 1.3, but it looks like it was close a month ago so it would be great if we could get this finished up and landed.

Comment 34

[Sam Joch [:samjoch]]

Comment on attachment 803783 [details]
New PR Github

I have updated the PR.

Comment 35

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 36

[Kevin Grandon :kgrandon]

Hi Sam,

Here's an approach with no JS changed which uses the new flex CSS stuff. Please let me know what you think! We can either land this or you can take it from here. Thanks!

Comment 37

[Kevin Grandon :kgrandon]

Comment on attachment 803783 [details]
New PR Github

I would like to explore the display: flex solution above. Please let me know what you think of the PR, clearing review flag for now.

Comment 38

[Sam Joch [:samjoch]]

Comment on attachment 803783 [details]
New PR Github

Kevin, thanks for you reco.
I think, that's definitively the right way to do it. i kept the .bottom class in order to have the shadow and add bottom: auto; to #view-selector.
Can you check my updated PR ?

Comment 39

[Kevin Grandon :kgrandon]

Comment on attachment 803783 [details]
New PR Github

Hi Sam, thanks for the patch, this looks good. I made one tiny update, and I've applied your changes on top of my original PR.

Comment 40

[Kevin Grandon :kgrandon]

Comment on attachment 8339057 [details] [review]
Github pull request

Already reviewed in different attachment.

Comment 41

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/fba3b72b9e5feb21128cfd97a4af656d0c0f0df7