Closed Bug 1583520 Opened 5 years ago Closed 5 years ago

remove grid usage from comm/calendar/resources/content/mouseoverPreviews.js

Categories

(Calendar :: General, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(5 files, 2 obsolete files)

No description provided.
Assignee: nobody → khushil324
Product: Thunderbird → Calendar
Attachment #9094898 - Flags: review?(paul)
Attachment #9094898 - Flags: feedback?(mkmelin+mozilla)

Create an event or a task and hover over it, you will see the popup with related information in a table format.

Status: NEW → ASSIGNED
Comment on attachment 9094898 [details] [diff] [review]
Bug-1583520_remove-grid-mouseoverPreviews-1.patch

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

Code changes look good and the preview panel looks good when I tried it.  I have some nit-picky naming requests.  It looks like all instances of "grid" in the JS file, except the first two, should be changed to "table" for consistency.

::: calendar/resources/content/mouseoverPreviews.js
@@ +352,3 @@
>   * @param  {Node}  box  The node to create a column grid for
>   */
>  function boxInitializeHeaderGrid(box) {

Rename to "boxInitializeHeaderTable" and replace "grid" with "table" in the function docsting.

@@ +367,2 @@
>   */
>  function boxAppendLabeledText(box, labelProperty, textString) {

Rename grid to table in docstring above this function.  And in similar docstrings in this file.

@@ +383,4 @@
>   * @returns {Node}         The node
>   */
>  function createTooltipHeaderLabel(text) {
> +  let header = document.createElementNS("http://www.w3.org/1999/xhtml", "th");

We've got two meanings for "header" going on here (1. the header of the panel which contains the table and 2. the th cell).  So let's rename this variable.  Maybe "labelCell"?

@@ +397,4 @@
>   * @returns {Node}         The node
>   */
>  function createTooltipHeaderDescription(text) {
> +  let description = document.createElementNS("http://www.w3.org/1999/xhtml", "td");

Maybe "descriptionCell" for parallel with "labelCell"?
Attachment #9094898 - Flags: review?(paul)
Attachment #9094898 - Attachment is obsolete: true
Attachment #9094898 - Flags: feedback?(mkmelin+mozilla)
Attachment #9094992 - Flags: review?(paul)
Comment on attachment 9094992 [details] [diff] [review]
Bug-1583520_remove-grid-mouseoverPreviews-2.patch

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

Looks good.  r+ with a couple of adjustments to naming in doc strings.

::: calendar/resources/content/mouseoverPreviews.js
@@ +5,4 @@
>  /**
>   * Code which generates event and task (todo) preview tooltips/titletips
>   *  when the mouse hovers over either the event list, the task list, or
> + *  an event or task box in one of the table views.

This should actually stay "grid views" because it's not talking about the table that's now in the mouseover preview, but some of the calendar views.  (I think I missed this in my previous review.)

@@ +19,4 @@
>  /**
>   * PUBLIC: This changes the mouseover preview based on the start and end dates
>   * of an occurrence of a (one-time or recurring) calEvent or calToDo.
> + * Used by all table views.

Same here: "grid views".
Attachment #9094992 - Flags: review?(paul) → review+
Attachment #9094992 - Attachment is obsolete: true
Attachment #9095301 - Flags: review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5e1904e992df
remove grid usage from mouseoverPreviews.js. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

No try run :-( - Seems to have caused bug 1584136 comment #5.

This is the patch from bug 1584136 which I'll land here.

Attachment #9095851 - Flags: review+
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---

Paul, you're not on IRC. Is that expected that testEventDialog.js throws an error at the end:
An error occurred when writing to the calendar Mozmill! Please see below for more information.

I think I've run this test before and it didn't show that.

Flags: needinfo?(paul)
Flags: needinfo?(geoff)

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3ef7c0d8bbba
Follow-up: fix testEventDialog.js and testTaskView.js. r=pmorris

Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 71

I looked and found that the error dialog occurs when the test dismisses the alarms. Here's a patch that adds a comment with more details about the error. I'm not sure what's causing it or how to prevent it, but at least it does not appear to affect the validity of the test results.

Flags: needinfo?(paul)
Attachment #9096639 - Flags: review?(geoff)
Flags: needinfo?(geoff)
Comment on attachment 9096639 [details] [diff] [review]
comment-about-event-dialog-test-error-0.patch

Yeah, this error shows up more frequently these days. I think there's another test with it too. It's mostly harmless – the result of the test moving at superhuman speed and not waiting for the previous modifications to save before making more.
Attachment #9096639 - Flags: review?(geoff) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/d9f793b6fcf4
Follow-up: Add comment about error dialog in testEventDialog.js. r=darktrojan
Comment on attachment 9095301 [details] [diff] [review]
Bug-1583520_remove-grid-mouseoverPreviews-3.patch

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

::: calendar/resources/content/mouseoverPreviews.js
@@ +398,5 @@
>   */
>  function createTooltipHeaderDescription(text) {
> +  let descriptionCell = document.createElementNS("http://www.w3.org/1999/xhtml", "td");
> +  descriptionCell.setAttribute("class", "tooltipHeaderDescription");
> +  descriptionCell.textContent = text;

Looks like this needs some more css. Now if you have a really long url (or whatever) in the description, the top part of the preview is... at the wrong place. 
Should make sure that the long strings wrap when appropriate. Maybe overflow-wrap: anywhere;

(In reply to Magnus Melin [:mkmelin] from comment #16)

Should make sure that the long strings wrap when appropriate. Maybe
overflow-wrap: anywhere;

Do you want to wrap the long description strings, right?

Something like this?

Yeah.

Attachment #9098929 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9098929 [details] [diff] [review]
Bug-1583520_follow-up_add-overflow-wrap-tooltipBody.patch

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

I think so
Attachment #9098929 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6ff5305dadb9
follow-up: add overflow-wrap to tooltipBody in mouseoverPreviews.js. r=mkmelin

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: