Closed Bug 951075 Opened 11 years ago Closed 11 years ago

[Calendar] Day View_1.4 Visual Refresh

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

VERIFIED FIXED
2.0 S2 (23may)
feature-b2g 2.0
tracking-b2g backlog

People

(Reporter: pekochen, Assigned: mmedeiros)

References

Details

(Whiteboard: [p=18])

Attachments

(10 files, 5 obsolete files)

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
20.72 KB, image/png
Details
No description provided.
Blocks: 950209
blocking-b2g: --- → backlog
Whiteboard: [p=5]
Assignee: nobody → mmedeiros
Target Milestone: --- → 1.4 S1 (14feb)
day view spec updated.
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)
Attached image icon_alarm.svg
uploaded alarm SVG format icon. but all day event icon still waiting for approvement.
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)
Blocks: 971923
Depends on: 972871
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)
Blocks: 951071
Target Milestone: 1.4 S1 (14feb) → 1.4 S2 (28feb)
Attached image 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 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-
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)
Attached image screenshot of PR (obsolete) —
screenshot with latest design
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
Hi Miller, Thanks for your great work~ I upload a sample image for your reference. thanks Peko
updated a detail spec. thanks
Attached image 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)
(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)
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.
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)
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)
Attached image event_sizes.png (obsolete) —
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)
Hi Miller, This looks great, and is just what Peko & I want! Thanks for the awesome work!
Flags: needinfo?(pchen)
Flags: needinfo?(hhsu)
Attachment #8377919 - Flags: ui-review?(pchen)
Attachment #8377919 - Flags: ui-review?(hhsu)
Attachment #8377919 - Flags: ui-review+
Whiteboard: [p=5] → [p=8]
Attached image 2014-02-27-12-04-19.png (obsolete) —
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)
Attached image 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)
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)
Attached image 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)
Ok Harly, will update the week view to hide text if events overlaps.
Flags: needinfo?(mmedeiros)
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)
Target Milestone: 1.4 S2 (28feb) → ---
Blocks: 993677
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
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)
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)
Attached image day-view-multiple.png
See behavior for multiple events in an hour
(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
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)
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
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)
Blocks: 963394
(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.
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
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+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [p=13] → [p=18]
No longer blocks: 963394
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
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: