[Calendar] Day View_1.4 Visual Refresh

VERIFIED FIXED in 2.0 S2 (23may)

Status

VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: pekochen, Assigned: mmedeiros)

Tracking

unspecified
2.0 S2 (23may)
ARM
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

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

Details

(Whiteboard: [p=18])

Attachments

(10 attachments, 5 obsolete attachments)

195.46 KB, application/pdf
Details
1.08 KB, image/svg+xml
Details
134.87 KB, image/png
Details
25.18 KB, image/png
Details
27.95 KB, image/png
Details
146.28 KB, image/png
Details
73.90 KB, image/png
Details
21.92 KB, image/png
Details
46 bytes, text/x-github-pull-request
gaye
: review+
harly
: ui-review+
Details | Review | Splinter Review
20.72 KB, image/png
Details
Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 950209
blocking-b2g: --- → backlog

Updated

5 years ago
Whiteboard: [p=5]
(Assignee)

Updated

5 years ago
Assignee: nobody → mmedeiros
Target Milestone: --- → 1.4 S1 (14feb)
(Reporter)

Comment 1

5 years ago
Created attachment 8372122 [details]
FFOS_VisualRefresh_1.4_calendar_dayview_spec.pdf

day view spec updated.
(Assignee)

Comment 2

5 years ago
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.
Flags: needinfo?(hhsu)
(Reporter)

Comment 3

5 years ago
Created attachment 8373875 [details]
icon_alarm.svg

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

Comment 4

5 years ago
Hi Miller,
I have Peko uploaded the icon as requested.
Please check and see if the SVG format work or not?
Thanks
Flags: needinfo?(hhsu)
(Assignee)

Updated

5 years ago
Blocks: 971923
(Assignee)

Updated

5 years ago
Depends on: 972871
(Assignee)

Comment 5

5 years ago
Created attachment 8377919 [details] [review]
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)
Attachment #8377919 - Flags: ui-review?(pchen)
Attachment #8377919 - Flags: ui-review?(hhsu)
Attachment #8377919 - Flags: review?(gaye)
(Assignee)

Updated

5 years ago
Blocks: 951071
(Assignee)

Updated

5 years ago
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
(Reporter)

Comment 6

5 years ago
Created attachment 8379602 [details]
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
(Reporter)

Comment 7

5 years ago
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~
Attachment #8377919 - Flags: ui-review?(pchen) → ui-review-
(Assignee)

Comment 8

5 years ago
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.
Attachment #8377919 - Flags: ui-review- → ui-review?(pchen)
(Assignee)

Comment 9

5 years ago
Created attachment 8380053 [details]
screenshot of PR

screenshot with latest design

Comment 10

5 years ago
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
(Reporter)

Comment 11

5 years ago
Created attachment 8380461 [details]
Calendar_v1.4_day_20140224

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

Peko
(Reporter)

Comment 12

5 years ago
Created attachment 8380501 [details]
Calendar_v1.4_day_spec_20140224

updated a detail spec.
thanks
(Assignee)

Comment 13

5 years ago
Created attachment 8380884 [details]
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.
Flags: needinfo?(pchen)
(Assignee)

Comment 14

5 years ago
(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?
Flags: needinfo?(hhsu)
(Assignee)

Comment 15

5 years ago
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

5 years ago
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?
Flags: needinfo?(hhsu)
(Reporter)

Comment 17

5 years ago
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
Flags: needinfo?(pchen)
(Assignee)

Comment 18

5 years ago
Created attachment 8381383 [details]
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.
Flags: needinfo?(pchen)
Flags: needinfo?(hhsu)

Comment 19

5 years ago
Hi Miller,
This looks great, and is just what Peko & I want! Thanks for the awesome work!
Flags: needinfo?(pchen)
Flags: needinfo?(hhsu)
(Assignee)

Updated

5 years ago
Attachment #8377919 - Flags: ui-review?(pchen)
Attachment #8377919 - Flags: ui-review?(hhsu)
Attachment #8377919 - Flags: ui-review+
(Assignee)

Updated

5 years ago
Whiteboard: [p=5] → [p=8]

Comment 20

5 years ago
Created attachment 8382754 [details]
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
Flags: needinfo?(mmedeiros)
(Assignee)

Comment 21

5 years ago
Created attachment 8383222 [details]
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.
Attachment #8381383 - Attachment is obsolete: true
Flags: needinfo?(mmedeiros) → needinfo?(hhsu)

Comment 22

5 years ago
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
Flags: needinfo?(hhsu)

Comment 23

5 years ago
Created attachment 8384456 [details]
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.
Flags: needinfo?(mmedeiros)
(Assignee)

Comment 24

5 years ago
Ok Harly, will update the week view to hide text if events overlaps.
Flags: needinfo?(mmedeiros)
(Assignee)

Comment 25

5 years ago
Created attachment 8387885 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16996
Attachment #8377919 - Attachment is obsolete: true
Attachment #8380053 - Attachment is obsolete: true
Attachment #8382754 - Attachment is obsolete: true
Attachment #8377919 - Flags: review?(gaye)
Attachment #8387885 - Flags: review?(gaye)
(Assignee)

Updated

5 years ago
Target Milestone: 1.4 S2 (28feb) → ---

Updated

5 years ago
Blocks: 993677
(Assignee)

Updated

5 years ago
Whiteboard: [p=8] → [p=13]
Target Milestone: --- → 2.0 S1 (9may)
blocking-b2g: backlog → 2.0+
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.
blocking-b2g: 2.0+ → backlog
(Assignee)

Comment 27

5 years ago
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.
Attachment #8387885 - Attachment is obsolete: true
Attachment #8387885 - Flags: review?(gaye)
(Assignee)

Comment 28

5 years ago
Created attachment 8415635 [details] [review]
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.
Attachment #8415635 - Flags: ui-review?(hhsu)
Attachment #8415635 - Flags: review?(gaye)
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)
Created attachment 8416777 [details]
day-view-multiple.png

See behavior for multiple events in an hour
(Assignee)

Comment 31

5 years ago
(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

5 years ago
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.
Flags: needinfo?(mmedeiros)
(Assignee)

Comment 33

5 years ago
just pushed some updates addressing some of Gareth's feedback and also updated the all day behavior. Let me know what you guys think.
Flags: needinfo?(mmedeiros) → needinfo?(hhsu)
feature-b2g: --- → 2.0

Comment 34

5 years ago
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.
Flags: needinfo?(hhsu)
(Assignee)

Updated

5 years ago
Blocks: 963394
(Assignee)

Comment 35

5 years ago
(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.
(Assignee)

Updated

5 years ago
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)

Comment 36

5 years ago
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!!
Attachment #8415635 - Flags: ui-review?(hhsu) → ui-review+
Comment on attachment 8415635 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/18847

r=me. Congrats Miller!
Attachment #8415635 - Flags: review?(gaye) → review+
(Assignee)

Comment 38

5 years ago
landed into master! https://github.com/mozilla-b2g/gaia/commit/c0d92f9e1c0580208ee83a7f2db01e56c2c21490
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=13] → [p=18]

Updated

5 years ago
No longer blocks: 963394
test case created : https://moztrap.mozilla.org/manage/case/13063/
Flags: in-moztrap+

Updated

5 years ago
Depends on: 1012796
[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
Status: RESOLVED → VERIFIED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 1002802
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.