remove grid usage from comm/calendar/resources/content/mouseoverPreviews.js
Categories
(Calendar :: General, task)
Tracking
(Not tracked)
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
darktrojan
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Create an event or a task and hover over it, you will see the popup with related information in a table format.
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
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"?
Assignee | ||
Comment 4•5 years ago
|
||
Comment 5•5 years ago
|
||
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".
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5e1904e992df
remove grid usage from mouseoverPreviews.js. r=pmorris
Comment 10•5 years ago
|
||
This is the patch from bug 1584136 which I'll land here.
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 12•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3ef7c0d8bbba
Follow-up: fix testEventDialog.js and testTaskView.js. r=pmorris
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 14•5 years ago
|
||
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.
Comment 15•5 years ago
|
||
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 16•5 years ago
|
||
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;
Assignee | ||
Comment 17•5 years ago
|
||
(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?
Comment 21•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Comment 22•5 years ago
|
||
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
Description
•