Closed Bug 1583157 Opened 5 months ago Closed 5 months ago

Category color overlaps reminder icon and inconsistent styling in week vs. month view

Categories

(Calendar :: Calendar Views, defect, minor)

Lightning 71
Unspecified
Windows
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ssitter, Assigned: Paenglab)

Details

Attachments

(4 files, 4 obsolete files)

Attached image category-color-view.png

Using Daily 71.0a1 (Build ID 20190922083552).

Setup multiple all-day/non-all-day events with/without reminder and with/without category. Check display in week view and month view.

Error:

  1. Category color is used as the background color for the reminder icon.
    -> based on the category color the icon might not be visible
    -> feels unbalanced when events with/without reminder are shown next to each other

  2. Styling in week view and month view is different, e.g.

  • all-day events in week view have a much larger padding on the right side compared to month view
  • non-all-day events in week view have a slightly larger padding on the right side compared to month view
  • category color is or is not used as the background color for the reminder icon

Proposal:
Do not fill reminder icon with category color, always use calendar color as background. Add like 1 px padding between reminder icon and category bar to avoid that the two elements connect visually. Remove the extra padding in week view and align styling between views.

Attached patch 1583157-category-color.patch (obsolete) — Splinter Review

Instead of a stack which puts one element above the other I use a hbox to put them beside.

Paul, please can you test the test too? I changed the selector and I do no tests locally.

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9097691 - Flags: review?(paul)
Comment on attachment 9097691 [details] [diff] [review]
1583157-category-color.patch

Review of attachment 9097691 [details] [diff] [review]:
-----------------------------------------------------------------

This fixes the problems in the month view (too much right padding and background color of reminder icon), and the calendar tests still pass.

It looks like this doesn't fix the problems in the week view yet, which is implemented in a different place, see: 
https://searchfox.org/comm-central/source/calendar/base/content/calendar-editable-item.js#161
Incidentally, here's another place I saw that seems similar. (I haven't looked at it further to see where it shows up in the UI.)
https://searchfox.org/comm-central/source/calendar/base/content/calendar-multiday-view.js#308
Attachment #9097691 - Flags: review?(paul)
Attached patch 1583157-category-color.patch (obsolete) — Splinter Review

This should be good now.

Attachment #9097691 - Attachment is obsolete: true
Attachment #9098365 - Flags: review?(paul)
Comment on attachment 9098365 [details] [diff] [review]
1583157-category-color.patch

Review of attachment 9098365 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!  Tests pass here.
Attachment #9098365 - Flags: review?(paul) → review+
Keywords: checkin-needed
Comment on attachment 9098365 [details] [diff] [review]
1583157-category-color.patch

Review of attachment 9098365 [details] [diff] [review]:
-----------------------------------------------------------------

Patch doesn't apply.

::: calendar/base/content/calendar-editable-item.js
@@ +168,5 @@
>                          </hbox>
> +                        <hbox class="calendar-category-box category-color-box calendar-event-selection"
> +                              flex="1" pack="end">
> +                            <image class="calendar-category-box-gradient">
> +                            </image>

Looks like the indentation isn't right here.
Keywords: checkin-needed

The indentation was already before my patch wrong (I haven't touched this line) -> fixed.

And updated to apply after bug 1585162.

Attachment #9098365 - Attachment is obsolete: true
Attachment #9098598 - Flags: review+
Keywords: checkin-needed

The indentation was already before my patch wrong (I haven't touched this line) -> fixed.

I know ;-)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ce3700fcd3c9
Don't stack the alarm-box and the category-box to not set the category color behind the alarm icon. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 71

In the month view and the today pane the event's category boxes don't have the correct height. I'm sure yesterday with the patch this didn't happen. Maybe something (blockification change in m-c ? ) changed this behaviour.

Attachment #9098930 - Flags: review?(paul)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

This is what it looks like with this latest patch on my build. Is this what it should look like? The height of the category swatch seems different than before.

Flags: needinfo?(richard.marti)
Comment on attachment 9098930 [details] [diff] [review]
1583157-fup-category-height.patch

Review of attachment 9098930 [details] [diff] [review]:
-----------------------------------------------------------------

Changes seem fine to me.  r+ assuming the screenshot I posted is what it's supposed to look like now, plus a white-space nit to fix.

::: calendar/base/themes/common/calendar-views.css
@@ +756,5 @@
>      font-weight: normal !important;
>      color: var(--viewMonthDayBoxWeekLabel) !important;
>  }
>  
> +

Looks like an extra empty line slipped in here.
Attachment #9098930 - Flags: review?(paul) → review+

Removed the empty line.

Attachment #9098952 - Attachment is obsolete: true
Flags: needinfo?(richard.marti)
Attachment #9098955 - Flags: review+
Keywords: checkin-needed
Attachment #9098952 - Attachment is obsolete: false
Attachment #9098930 - Attachment is obsolete: true

(In reply to Paul Morris [:pmorris] from comment #10)

Created attachment 9098952 [details]
category-color-month-view.png

This is what it looks like with this latest patch on my build. Is this what it should look like? The height of the category swatch seems different than before.

This is without the follow-up patch? Then yes. With the follow-up patch the category box should be taller. I'm building now on Linux to check.

(In reply to Richard Marti (:Paenglab) from comment #13)

This is without the follow-up patch? Then yes. With the follow-up patch the category box should be taller. I'm building now on Linux to check.

Hm, that screenshot is with the follow-up patch. Without the follow up patch I didn't see any category box at all. So maybe Linux is special?

Okay, let's see when my build finished.

Keywords: checkin-needed

My previous approach was wrong, it worked only when a alarm was set. This one should be better.

Attachment #9098955 - Attachment is obsolete: true
Attachment #9098965 - Flags: review?(paul)
Comment on attachment 9098965 [details] [diff] [review]
1583157-fup-category-height.patch

Review of attachment 9098965 [details] [diff] [review]:
-----------------------------------------------------------------

These changes seem fine.  I wasn't able to test it out because the first patch (1583157-category-color.patch) no longer applies cleanly to trunk.  So that will need to be updated.
Attachment #9098965 - Flags: review?(paul)
Comment on attachment 9098965 [details] [diff] [review]
1583157-fup-category-height.patch

1583157-category-color.patch was already checked-in. No need to apply it again.
Attachment #9098965 - Flags: review?(paul)
Attachment #9098598 - Attachment description: 1583157-category-color.patch → 1583157-category-color.patch checked-in comment 8
Comment on attachment 9098965 [details] [diff] [review]
1583157-fup-category-height.patch

Review of attachment 9098965 [details] [diff] [review]:
-----------------------------------------------------------------

Ahhh... of course!  OK, I've tested this and the category indicator looks good now.
Attachment #9098965 - Flags: review?(paul) → review+

Thanks.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b4da7ba1ebe0
Follow-up: Fix the category-box height in month view. r=pmorris

Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.