remove grid usage from comm/chat/content/chat-tooltip.js
Categories
(Thunderbird :: Instant Messaging, task)
Tracking
(Not tracked)
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(1 file, 3 obsolete files)
3.95 KB,
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 2•5 years ago
|
||
Comment on attachment 9094168 [details] [diff] [review] Bug-1582424_remove-grid-chat-tooltip.patch Review of attachment 9094168 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/chat-tooltip.js @@ +289,5 @@ > label.setAttribute("value", aLabel); > + let td1 = document.createElementNS( > + "http://www.w3.org/1999/xhtml", > + "td" > + ); looks like the first one should be <th> But don't add a <label> under them, just add the content (createTextNode)
Assignee | ||
Comment 3•5 years ago
•
|
||
Should we also remove the <description> tag?
Comment 4•5 years ago
|
||
Yeah please remove that too
Assignee | ||
Comment 5•5 years ago
|
||
Comment 6•5 years ago
|
||
Comment on attachment 9094213 [details] [diff] [review] Bug-1582424_remove-grid-chat-tooltip.patch Review of attachment 9094213 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/chat-tooltip.js @@ +287,5 @@ > + let th = document.createElementNS("http://www.w3.org/1999/xhtml", "th"); > + th.textContent = aLabel; > + row.appendChild(th); > + let td = document.createElementNS("http://www.w3.org/1999/xhtml", "td"); > + description = td; seems a bit unnecesary to have the let td here. Just do descriptition = document.createElementNS("http://www.w3.org/1999/xhtml") @@ +297,5 @@ > } > description.textContent = aValue; > } > > addSeparator() { Didn't spot this earlier, but the addSeparator() should be redone, instead of adding an "empty" row. Like in the other bug, add a class to style as needed.
Assignee | ||
Comment 7•5 years ago
|
||
Comment 8•5 years ago
|
||
Comment on attachment 9094879 [details] [diff] [review] Bug-1582424_remove-grid-chat-tooltip.patch Review of attachment 9094879 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, r=mkmelin ::: chat/content/chat-tooltip.js @@ +213,5 @@ > <description class="tooltipMessage"></description> > </stack> > </hbox> > + <table xmlns="http://www.w3.org/1999/xhtml" class="tooltipTable"> > + </table> maybe just <html:table class="tooltipTable"></table> Everything beneath it will be html sure, but since you're creating through script, it doesn't matter.
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Comment on attachment 9094892 [details] [diff] [review] Bug-1582424_remove-grid-chat-tooltip.patch Review of attachment 9094892 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/content/chat-tooltip.js @@ +300,5 @@ > > addSeparator() { > + if (this.table.hasChildNodes()) { > + let lastElement = this.table.lastChild; > + lastElement.querySelector("th").classList.add("chatTooltipSeparator"); Would it make sense to add this to the `tr` (which is likely `lastElement`), instead of the `th` and `td` separately?
Assignee | ||
Comment 11•5 years ago
•
|
||
(In reply to Patrick Cloke [:clokep] from comment #10)
Would it make sense to add this to the
tr
(which is likelylastElement
),
instead of theth
andtd
separately?
html:tr does not take border property i.e. we can not apply border style to tr directly.
Comment 12•5 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #11)
(In reply to Patrick Cloke [:clokep] from comment #10)
Would it make sense to add this to the
tr
(which is likelylastElement
),
instead of theth
andtd
separately?html:tr does not take border property i.e. we can not apply border style to tr directly.
Makes sense! Thanks!
Comment 13•5 years ago
|
||
Please check the linting before submitting a patch. I've fixed it for you here.
Comment 14•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/398e5adc18c3
remove grid usage from chat-tooltip.js. r=mkmelin
Updated•5 years ago
|
Description
•