951071 - [Calendar] Week 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_weekview_spec.pdf

week view visual spec updated.

Comment 2

[Miller Medeiros [:millermedeiros]]

Attachment: week_01.png

I'm having some trouble cropping the text if it's longer than the container and event lasts longer than 1h (see event starting at 2AM on Mon 24). Is it OK if we keep it like this? (since it's an edge case). All the solutions for this problem that I could think of are "hackish" and/or require JavaScript.

I'm doing the development on this branch (https://github.com/millermedeiros/gaia/tree/951071-week-view-visual-refresh) if you guys want to test it before I finish polishing it. I should do a pull request tomorrow, still need to write tests for it and double check my changes.

Let me know if you guys see anything that is looking wrong. Thanks.

Comment 3

[Harly Hsu[:harly]]

Hi Miller,
The text cropping method seems to be different of what we expected. We are thinking that the title should be considered as a string, so system will get in as much as text as possible. For instance, "Awesome person event" in the screenshot should not be 
"Aweso...
perso...
event"
and should be
"Awesom
e perso
n event
"

And about the trouble of cropping text if it's longer than the container, is there a way we could avoid this? It is not visually appealing to see text get cropped.

Comment 4

[Miller Medeiros [:millermedeiros]]

Hi Harly, unfortunately the way CSS "text-overflow:ellipsis" works is not flexible enough. If we set "word-wrap:break-word" or "word-break:break-all" we lose the ellipsis. The ellipsis only works for text that uses a single line. Opera has a special "text-overflow" value called "-o-ellipsis-lastline", but gecko doesn't have anything similar.

I'm going to try to find a way to avoid cropping the lines in the middle, but I'm almost sure there will be edge cases unless we use JavaScript and/or make the markup structure way more complex (which could degrade performance).

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

There's the visual fade-out trick like at http://css-tricks.com/line-clampin/.  Note that a fade-out is key versus just displaying ellipsis because the fade-out does nothing if you don't actually make it all the way to where the fade-out is.

Comment 6

[Miller Medeiros [:millermedeiros]]

The hard thing about the fade, is that right now the background color of the events uses transparency (0.2 alpha); so if add another layer on top of it, the color won't match. Would need to convert color to HSL and change lightness until it's "close enough", but I guess that might be the "simplest" solution.

There are some techniques like http://www.mobify.com/blog/multiline-ellipsis-in-pure-css/ but it only solves the horizontal overflow (won't help with the cropping text) and only works with fixed height. #cssishard

Comment 7

[Miller Medeiros [:millermedeiros]]

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

added integration tests and kept same behavior as shown in the screenshot https://bugzilla.mozilla.org/attachment.cgi?id=8383326 - only difference is that now we hide the event title if it "overlaps" with another event (because it's too narrow to display properly)

I would suggest merging it as is and looking for a way to improve the text cropping as a separate "polish" issue in the future. It is already WAY better than current behavior and it's an edge case.

Comment 8

[Miller Medeiros [:millermedeiros]]

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

Comment 9

[Harly Hsu[:harly]]

Attachment: device-2014-03-10-101203.png

Hi Miller,
After updating the patch, I found that the all day event in week view are not displayed properly.
Could you take a look at it? Thanks

Comment 10

[Miller Medeiros [:millermedeiros]]

going to take a look when we start the v1.5 sprints! abandoned the visual refresh for now.

Comment 12

[Miller Medeiros [:millermedeiros]]

we should highlight the "today date" on this patch as well since I marked Bug 963428 as a duplicate. (will be easier to review and land it).

Comment 13

[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 14

[Miller Medeiros [:millermedeiros]]

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

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

Comment 15

[Miller Medeiros [:millermedeiros]]

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

Harly and Gareth, I rebased and updated the week view patch. Also fixed the all day events and did some polish work. I still need to update the integration tests to match the new test format, will work on it tomorrow. But I would like to get a design/functionality feedback as soon as possible, that's why I'm submitting the patch before doing it. Thanks!

PS: this patch depends on the day view, so it includes changes for both.

Comment 16

[Harly Hsu[:harly]]

Attachment: Slide1.png

Hi Miller, below are some comments about the patch:
1. The color representing different accounts changes when tapping refresh
2. The event space should be recalculated when modifying the selection of accounts to display in drawer(see attached image for details)
3. The text cropping issue on comment3, is there a way to do this?
4. Limit title of all day event to just single line
5. It seems that the lines are too light and hard to see; therefore, change lines from #f8f8f8 to #f1f1f1
6. The text of Time should be regular and the text of event title should be medium

Comment 17

[Miller Medeiros [:millermedeiros]]

(In reply to Harly Hsu from comment #16)
> Created attachment 8417845 [details]
> Slide1.png
> 
> Hi Miller, below are some comments about the patch:
> 1. The color representing different accounts changes when tapping refresh

yes, I need to persist the colors! Gareth pointed it out on the Day view patch as well. - same goes if you add an account, close the app and add a new account.

> 2. The event space should be recalculated when modifying the selection of
> accounts to display in drawer(see attached image for details)

this is not possible with the current structure, I'm almost sure we had a dedicated issue for this but I could not find it. - I think we should do it as a separate patch (it affects the whole structure, it is not a simple visual change).

> 3. The text cropping issue on comment3, is there a way to do this?

there is not an "easy" way to crop the text, CSS/HTML have limitations. If we use "text-overflow: ellipsis" it adds the "..." at each line, and if we use "word-wrap: break-word" it will still add more line breaks than what you would expect. - my vote is to keep it as is, since it shows as much text as possible (will be easier for the user to understand what is that event).

It is really tricky to fix the vertical cropping, we can't use a gradient because the background color does not have 100% opacity (adding another layer on top of it wouldn't look good). So I think this is the best that we can do at the moment.

> 4. Limit title of all day event to just single line

Ok.

> 5. It seems that the lines are too light and hard to see; therefore, change
> lines from #f8f8f8 to #f1f1f1

Ok.

> 6. The text of Time should be regular and the text of event title should be
> medium

Ok.

Comment 18

[Harly Hsu[:harly]]

(In reply to Miller Medeiros [:millermedeiros] from comment #17)
> (In reply to Harly Hsu from comment #16)
> > Created attachment 8417845 [details]
> > Slide1.png
> > 
> > Hi Miller, below are some comments about the patch:
> > 1. The color representing different accounts changes when tapping refresh
> 
> yes, I need to persist the colors! Gareth pointed it out on the Day view
> patch as well. - same goes if you add an account, close the app and add a
> new account.
> 
> > 2. The event space should be recalculated when modifying the selection of
> > accounts to display in drawer(see attached image for details)
> 
> this is not possible with the current structure, I'm almost sure we had a
> dedicated issue for this but I could not find it. - I think we should do it
> as a separate patch (it affects the whole structure, it is not a simple
> visual change).
> 
Miller, I have found bug 937307 related to this. And this will affect the overall usability of calendar. I will ask Candice & Wilfred to add it to our backlog.

> > 3. The text cropping issue on comment3, is there a way to do this?
> 
> there is not an "easy" way to crop the text, CSS/HTML have limitations. If
> we use "text-overflow: ellipsis" it adds the "..." at each line, and if we
> use "word-wrap: break-word" it will still add more line breaks than what you
> would expect. - my vote is to keep it as is, since it shows as much text as
> possible (will be easier for the user to understand what is that event).
> 
> It is really tricky to fix the vertical cropping, we can't use a gradient
> because the background color does not have 100% opacity (adding another
> layer on top of it wouldn't look good). So I think this is the best that we
> can do at the moment.
OK, in that case, I think what we have right now is acceptable.
> 
> > 4. Limit title of all day event to just single line
> 
> Ok.
> 
> > 5. It seems that the lines are too light and hard to see; therefore, change
> > lines from #f8f8f8 to #f1f1f1
> 
> Ok.
> 
> > 6. The text of Time should be regular and the text of event title should be
> > medium
> 
> Ok.

Comment 19

[Harly Hsu[:harly]]

Attachment: device-2014-05-13-121002.png

Miller, in week view, it has the same issue as day view that if you have multiple all day events, when scroll up/down the all day events, it seems to be jittering left and right a little. Also in week view that has four days displayed, I am able to scroll left/right, and you can see a horizontal scroll bar.

Comment 20

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

https://moztrap.mozilla.org/manage/case/13107/

Test case will need to be updated accordingly.

Comment 21

[Miller Medeiros [:millermedeiros]]

(In reply to Harly Hsu from comment #19)
> Created attachment 8421491 [details]
> device-2014-05-13-121002.png
> 
> Miller, in week view, it has the same issue as day view that if you have
> multiple all day events, when scroll up/down the all day events, it seems to
> be jittering left and right a little. Also in week view that has four days
> displayed, I am able to scroll left/right, and you can see a horizontal
> scroll bar.

Harly, I just updated the patch and fixed the jittering (was caused by border around the scrollable area - I still need to report the bug to the APZ team), and also fixed the horizontal scroll (was caused by padding on the events).

Comment 22

[Harly Hsu[:harly]]

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

Nice work, Miller. Thanks for the hard work.

Comment 23

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

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

Thanks again Miller!

Comment 24

[Miller Medeiros [:millermedeiros]]

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

Comment 25

[Francesco Lodolo [:flod]]

Hi, I'm your nightmare ;-)

+hour-format-abbr-am=%IA
+hour-format-abbr-pm=%IP

I think we have a problem with these design: some locales localize AM/PM. 
http://transvision.mozfr.org/string/?entity=shared/date/date.properties:time_am&repo=gaia
http://transvision.mozfr.org/string/?entity=shared/date/date.properties:time_pm&repo=gaia

And I'm not sure they can abbreviate this to one character. What happens if instead of "A" you have "පෙ.ව."?

In any case: please add a localization note explaining that the time code is %I, A and P are abbreviations for AM/PM. My first reaction was: what kind of code is %IA?

Comment 26

[Francesco Lodolo [:flod]]

Also (sorry for the double comment): what am I supposed to do if my locales uses 24h format? Because that key name is quite confusing.

Comment 27

[Miller Medeiros [:millermedeiros]]

for 24h we should just use the same value for both keys. I'm not sure what to do with languages that can't abbreviate the "AM/PM", my initial guess was to just use "%I" (since most people won't understand the 24h format).

Harly, do you have any feedback about the localization from the UX team? What should we do?

Comment 28

[Harly Hsu[:harly]]

I think what our initial design is to use A for AM and P for PM across all languages. The reason for this is that since most of our device's screen size is limited, we don't really have enough space to display text like "පෙ.ව." without truncation. And I think AM/PM is used generally across all languages in clocks and watches, and if you take a look at Blackberry, they use AM/PM in calendar for all languages as well. I am not a localization expert, so I need to consult Francesco that is it OK to use AM/PM across all languages? Thanks

Comment 29

[Francesco Lodolo [:flod]]

(In reply to Harly Hsu from comment #28)
> I am not a localization expert, so I need to consult Francesco that is it OK to 
> use AM/PM across all languages?

It is not. See links in comment 25 for the current translations across all languages.

See also bug 968062 and dependencies: it's not just about localizing AM/PM, but also about showing them before the hour.

Comment 30

[Harly Hsu[:harly]]

Need info visual designer Peko for modifications on having AM/PM on different languages to the visual spec.

Comment 31

[Peko Chen [:pekochen]]

Attachment: FFOS_VisualRefresh_2.0_calendar_weekview_spec_20140602.pdf

Hi, I have updated the UI spec for localization for week view. Basically AM/PM will be separated from time and break into a second row, and the font size for AM/PM will be 13px to fit the "පෙ.ව.".

Comment 32

[Edward Chen[:edchen]]

Attachment: Bug screenshoot

Spec has been modified, I reopen this ticket and please resolve it to fulfill new specification.   

[Actual screenshot] 
The AM/PM does displays A/P.

Comment 33

[Jason Smith [:jsmith]]

Can open a followup bug for the issue you are seeing?

Comment 34

[Miller Medeiros [:millermedeiros]]

I'm already working on this, so I will open a new bug for it.

Comment 35

[Miller Medeiros [:millermedeiros]]

new bug to track the am/pm L10n: https://bugzilla.mozilla.org/show_bug.cgi?id=1019617

Comment 36

[Edward Chen[:edchen]]

[Environment]
Gaia      8d865839d932bfbd5e157f376f74d8cb12bfdd51
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/1d4046a8cb6c
BuildID   20140610000223
Version   32.0a2
ro.build.version.incremental=94
ro.build.date=Tue May 20 09:29:20 CST 2014

[Result]
Pass