951075 - [Calendar] Day View_1.4 Visual Refresh

Status: VERIFIED FIXED

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

Comment 1

[Peko Chen [:pekochen]]

Attachment: FFOS_VisualRefresh_1.4_calendar_dayview_spec.pdf

day view spec updated.

Comment 2

[Miller Medeiros [:millermedeiros]]

can anyone provide the icons needed for the visual changes? (all day event & alarm). It would be great if icons were in SVG or a custom font (vectors), that way we can change the colors as needed and don't need to worry about different resolutions. - SVG is easier to handle.

Comment 3

[Peko Chen [:pekochen]]

Attachment: icon_alarm.svg

uploaded alarm SVG format icon.
but all day event icon still waiting for approvement.

Comment 4

[Harly Hsu[:harly]]

Hi Miller,
I have Peko uploaded the icon as requested.
Please check and see if the SVG format work or not?
Thanks

Comment 5

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16426

updated the design of the day view to match latest specs.

please note that the "blue line" marking the current hour is going to be implemented as a separate patch. (Bug 971923)

Comment 6

[Peko Chen [:pekochen]]

Attachment: review_dayview.png

please check attached file for details.
1. All day event background color should be # f8f8f8
2. Move up 2 pixel
3. Remove white line
4. Font : 15px ,#707070 , regular
5. It’s not a complete rectangle
6. Font: Fira Sans, 14px,Medium,#333333 (title)
   Font: Fira Sans, 13px,Regular,#707070 (location)
7. event color:
8.tab font :
   normal: regular ; selected : medium

Comment 7

[Peko Chen [:pekochen]]

Comment on attachment 8377919 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16426

please check previous attachment for visual feedback.
thanks~

Comment 8

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8377919 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16426

updated the pull request based on visual feedback. It should look way better now. I fixed some of the errors before seeing the comments but I probably pushed to github after you started to review. (sorry about that)

I implemented the color in a way that the orange is always for the "Offline Calendar" and looped through the other colors for the remote calendars. I loop in this order: https://github.com/millermedeiros/gaia/blob/951075-day-view-visual-refresh/apps/calendar/js/provider/caldav.js#L43-L53 - that way most users will have colors that are distinct enough for all the calendars. If the user have more than 7 remote calendars we loop the colors following the same order (start again at light blue) - probably won't happen tho.

It's important to notice that caldav (remote calendars) usually contain their own "color", we are ignoring the information from the server and overriding this value. Also good to notice that most calendars ignore this value as well. (so we are probably good)

I had added the white line between events because if you have 1 event just after another, and they start/end before a full hour, it looks like both events are the "same" (no clear division between them). But removed now since I agree it doesn't look as good in other cases. (it was also causing the container to not look like a rectangle)

I created a new bug dedicated for the "tabs", since that is not technically part of the day view (Bug 975617).

some notes:

 - improved the way multiple consecutive events are displayed, if more than 5 events I hide the copy (since it doesn't make sense).
 - alarm icon and location are hidden if event lasts less than 1hour.
 - still need to find a solution if events is smaller than 30min (Bug 962445)

let me know if there is anything that should be improved on the day view. thanks.

Comment 9

[Miller Medeiros [:millermedeiros]]

Attachment: screenshot of PR

screenshot with latest design

Comment 10

[Harly Hsu[:harly]]

Hi Miller,
Great work!
After making offline discussion with Peko, here are some items that could be improved:
- Remove the white line between event if an event follow another, and we would like to solve the issue by reducing 1px from top & bottom of each event.
- Alarm icon and text are too close, could the margin between text & alarm icon be the same as alarm to the right border?
- Set the minimum event hight to 30 min, if event is less than 30 min, it will look the same as 30 min event.
- Alarm icon will not appear when event time is less than 30 min
- Event name & location will be displayed in two lines when event time is more than 30 min, if event time is less than 30 min, event name & location will be displayed in a single line if space is available.

Thanks

Comment 11

[Peko Chen [:pekochen]]

Attachment: Calendar_v1.4_day_20140224

Hi Miller,
Thanks for your great work~
I upload a sample image for your reference.
thanks

Peko

Comment 12

[Peko Chen [:pekochen]]

Attachment: Calendar_v1.4_day_spec_20140224

updated a detail spec.
thanks

Comment 13

[Miller Medeiros [:millermedeiros]]

Attachment: day_20140224_2.png

I think that a busy calendar wont looks so nice with a 1px margin before and after the events (image at the left). Maybe we should just reduce the event height by 1px (image on the right).

also notice that the current hour line is almost same color as the blue events, maybe we should change the hue/saturation a little bit.

Comment 14

[Miller Medeiros [:millermedeiros]]

(In reply to Harly Hsu from comment #10)
> - Set the minimum event hight to 30 min, if event is less than 30 min, it
> will look the same as 30 min event.

There are two bugs about it (Bug 830180 and Bug 862102). If we limit the height to cover 30min we will introduce bugs (2 consecutive 15min events will overlap), so I would not recommend doing it together with this patch. I would probably land the visual refresh and improve it later (easier to review it and revert in cases things goes wrong).

I was initially planning to hide the copy if we don't have enough room to display it, would that be enough for now?

Comment 15

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8377919 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16426

I just pushed a new commit to the pull request based on latest design feedback.

Comment 16

[Harly Hsu[:harly]]

Hi Miller,
Sorry, I don't quite understand the term "hide the copy" means.
Do you mean that if the event do not have enough room to display text, we will just not show it? 
If that's the case, I agree it is better than to show the truncated text.

And about the patch, name & location at same line if event is less than 1h, could this be changed to less than 45 min instead of 1h?

Comment 17

[Peko Chen [:pekochen]]

Hi Miller,

yeah, I agree with your suggestion, let's keep just reduce the event height by 1px.
for current time line, can you please change color to #0091ae ?
thanks for your help.

Peko

Comment 18

[Miller Medeiros [:millermedeiros]]

Attachment: event_sizes.png

I just did a quick test and I think that's the best we can do for now:

 - events smaller than 1h we reduce the padding around title/location.
 - if <45min put text at same line and center the alarm icon.
 - if <30min hide icon.
 - if <20min hide title/location.

let me know what you think.

Comment 19

[Harly Hsu[:harly]]

Hi Miller,
This looks great, and is just what Peko & I want! Thanks for the awesome work!

Comment 20

[Harly Hsu[:harly]]

Attachment: 2014-02-27-12-04-19.png

Hi Miller,
Just found out that if there is no reminder set for calendar event, there will still be a blank spot. Is it possible to fill-in the text if no reminder is set?
Thanks

Comment 21

[Miller Medeiros [:millermedeiros]]

Attachment: day_view_refresh.png

Updated the pull request. Now the text container width is toggled if alarm icon is displayed; if box is too small to show icon, or no reminder, we use whole space.

Yesterday I also centered the text vertically if it's displayed in a single line, it was looking weird depending on the event duration.

The browser is handling the text cropping (ellipsis) automatically; if event title and location have about the same amount of characters they should have around the same width. If title is longer it uses more space, but you should still be able to see the beginning of the location. - I like this behavior a lot.

Let me know if you guys want me to change anything else. Cheers.

Comment 22

[Harly Hsu[:harly]]

Hi Miller,
About text centered vertically in single line, do you mean that the 25min event Sao Paulo look weird? What I've seen from your screenshot, the Sao Paulo looks a little bit higher. Other than that, everything seems to look fine.

The browser handling method you've mentioned, after discussing with Peko, we think that Title is more important than location. Therefore, I think what we have right now is OK.
Thanks

Comment 23

[Harly Hsu[:harly]]

Attachment: 2014-03-03-15-36-12.png

Miller, another thing I've noticed is if two or more events share a same container in week view, like the screenshot, I think it will be better not to show any text.

Comment 24

[Miller Medeiros [:millermedeiros]]

Ok Harly, will update the week view to hide text if events overlaps.

Comment 25

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16996

Comment 26

[Jason Smith [:jsmith]]

This shouldn't block - this is a feature, which is something we won't block on unless we're past FL & we're planning to still land the feature.

Comment 27

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8387885 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16996

closed the old pull request since it was targeting the wrong branch. will submit a new PR later this week.

Comment 28

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18847

I updated the day view pull request; rebased over master, fixed a few bugs and did some polish. Please let me know if you guys have any feedback.

Harly, I updated the "all day" events scroll as well. It is not exactly what you suggested on Bug 1002802 but I think it is working "well enough". I tried to set a max-height for the all-day (display up to 2 events) but it broke the hours scroll, so I gave up for now. - maybe that is something we can improve later.

PS: the "day view" affects the "month" and "week" views, please ignore any weird artifacts on these cards since they will also be updated during the visual refresh.

Comment 29

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

Looks really good! Nice work :). I've played around with it a bit and so far I've found that

(1) only the first all day event on a given day is shown
(2) when an event is shorter than an hour, instead of being rendered below the title, the location is rendered to the right of the title. Is this what we want? (Not saying it's incorrect just noting)

Comment 30

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

Attachment: day-view-multiple.png

See behavior for multiple events in an hour

Comment 31

[Miller Medeiros [:millermedeiros]]

(In reply to Gareth Aye [:gaye] from comment #29)
> Looks really good! Nice work :). I've played around with it a bit and so far
> I've found that
> 
> (1) only the first all day event on a given day is shown

you can scroll the all day events, on Bug 1002802 Harly suggested to show up to 2 events before scrolling but that is really tricky to implement with just CSS.

> (2) when an event is shorter than an hour, instead of being rendered below
> the title, the location is rendered to the right of the title. Is this what
> we want? (Not saying it's incorrect just noting)

yes, that is something that we decided to do, see: https://bugzilla.mozilla.org/show_bug.cgi?id=951075#c18

Comment 32

[Harly Hsu[:harly]]

Hi Miller, great work on the day view. Here are some feedbacks:
1. The all day icon will move when scrolling through multiple all day events. The icon should be fixed and not scrollable
2. About multiple all day events, it is hard for the user to know that there are multiple events as only one is being seen. Therefore, could we change the height of all day event from 1hr to 45min? And when only 1 all day event is available, display the event vertically center aligned to the all day area.

Comment 33

[Miller Medeiros [:millermedeiros]]

just pushed some updates addressing some of Gareth's feedback and also updated the all day behavior. Let me know what you guys think.

Comment 34

[Harly Hsu[:harly]]

Miller, nicely done, just that could you align the all day event vertically center when there is only one all day event? Also, when I scroll up/down the all day events, it seems to be jittering left and right a little.

Comment 35

[Miller Medeiros [:millermedeiros]]

(In reply to Harly Hsu from comment #34)
> Miller, nicely done, just that could you align the all day event vertically
> center when there is only one all day event? Also, when I scroll up/down the
> all day events, it seems to be jittering left and right a little.

Harly, I fixed both issues on my latest commit.. I also addressed all feedback from Gareth. Can't wait to land all the visual refresh patches! cheers.

Comment 36

[Harly Hsu[:harly]]

Comment on attachment 8415635 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18847

Looks great, thanks for the great work!!

Comment 37

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

Comment on attachment 8415635 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18847

r=me. Congrats Miller!

Comment 38

[Miller Medeiros [:millermedeiros]]

landed into master! https://github.com/mozilla-b2g/gaia/commit/c0d92f9e1c0580208ee83a7f2db01e56c2c21490

Comment 39

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

test case created : https://moztrap.mozilla.org/manage/case/13063/

Comment 40

[Edward Chen[:edchen]]

[Environment]
Gaia      8a2352d5b7be27ec4b1ea18c680ebcd0b6d34348
Gecko     https://hg.mozilla.org/mozilla-central/rev/cb9f34f73ebe
BuildID   20140519160202
Version   32.0a1
ro.build.version.incremental=324
ro.build.date=Thu Dec 19 14:04:55 CST 2013

[Result]
PASS