[Calendar] Week 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=13])

Attachments

(7 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 950209
See Also: → bug 885946
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 8372120 [details]
FFOS_VisualRefresh_1.4_calendar_weekview_spec.pdf

week view visual spec updated.
(Assignee)

Updated

5 years ago
Blocks: 971923
(Assignee)

Updated

5 years ago
Depends on: 972871
(Assignee)

Updated

5 years ago
Depends on: 951075
(Assignee)

Updated

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

Comment 2

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

Comment 3

5 years ago
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.
Flags: needinfo?(hhsu)
Target Milestone: 1.4 S2 (28feb) → ---
(Assignee)

Comment 4

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

Comment 6

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

Comment 7

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

Updated

5 years ago
Blocks: 963428
(Assignee)

Comment 8

5 years ago
Created attachment 8387915 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16997
Attachment #8387652 - Attachment is obsolete: true
Attachment #8387652 - Flags: review?(gaye)
Attachment #8387915 - Flags: review?(gaye)

Comment 9

5 years ago
Created attachment 8388280 [details]
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
Flags: needinfo?(mmedeiros)
going to take a look when we start the v1.5 sprints! abandoned the visual refresh for now.
Flags: needinfo?(mmedeiros)
Blocks: 993677
(Assignee)

Updated

5 years ago
No longer blocks: 963428
Duplicate of this bug: 963428
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).
(Assignee)

Updated

5 years ago
Whiteboard: [p=5] → [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 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.
Attachment #8387915 - Attachment is obsolete: true
Attachment #8387915 - Flags: review?(gaye)
Created attachment 8416168 [details] [review]
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.
Attachment #8388280 - Attachment is obsolete: true
Attachment #8416168 - Flags: ui-review?(hhsu)
Attachment #8416168 - Flags: review?(gaye)
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
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
Flags: needinfo?(mmedeiros)
(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.
Flags: needinfo?(mmedeiros)
(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.
feature-b2g: --- → 2.0
(Assignee)

Updated

5 years ago
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
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.
Flags: needinfo?(mmedeiros)
https://moztrap.mozilla.org/manage/case/13107/

Test case will need to be updated accordingly.
Flags: in-moztrap+
(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).
Flags: needinfo?(mmedeiros)
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.
Attachment #8416168 - Flags: ui-review?(hhsu) → ui-review+
(Assignee)

Updated

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

Thanks again Miller!
Attachment #8416168 - Flags: review?(gaye) → review+
finally landed into master! https://github.com/mozilla-b2g/gaia/commit/a42331da11190c75fe783171ec263f4b621e841a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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?
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.
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?
Flags: needinfo?(hhsu)
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
Flags: needinfo?(hhsu) → needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
Need info visual designer Peko for modifications on having AM/PM on different languages to the visual spec.
Flags: needinfo?(pchen)
QA Contact: edchen
(Reporter)

Comment 31

5 years ago
Created attachment 8433021 [details]
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 "පෙ.ව.".
Flags: needinfo?(pchen)
Created attachment 8433182 [details]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can open a followup bug for the issue you are seeing?
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: needinfo?(edchen)
Resolution: --- → FIXED
I'm already working on this, so I will open a new bug for it.
Flags: needinfo?(edchen)
(Assignee)

Updated

5 years ago
Depends on: 1019617
[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
Status: RESOLVED → VERIFIED
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.