Closed
Bug 1568189
Opened 5 years ago
Closed 5 years ago
remove grid usage from comm/calendar/base/content/calendar-task-view.xul
Categories
(Thunderbird :: General, task)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 70.0
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(1 file, 9 obsolete files)
|
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Updated•5 years ago
|
Assignee: nobody → khushil324
|
|
Assignee | |
Comment 1•5 years ago
|
||
Attachment #9079991 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Updated•5 years ago
|
Blocks: tb-burn-xul-grids
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 2•5 years ago
|
||
Attachment #9079991 -
Attachment is obsolete: true
Attachment #9079991 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9080283 -
Flags: feedback?(mkmelin+mozilla)
|
||
Comment 3•5 years ago
|
||
Comment on attachment 9080283 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view.patch
Review of attachment 9080283 [details] [diff] [review]:
-----------------------------------------------------------------
::: calendar/base/content/calendar-task-view.js
@@ +114,4 @@
> let taskStartRowLabel = document.getElementById("task-start-row-label");
> let taskStartDate = item[cal.dtz.startDateProp(item)];
> taskStartRowLabel.style.visibility = taskStartDate ? "visible" : "collapse";
> + taskStartRowLabel.parentNode.parentNode.setAttribute("hidden", taskStartDate ? "true" : "false");
hmm, that's an html element right? hidden is a boolean attribute there so it's hidden="hidden" or no hidden at all.. But you can set the hidden property to true or false
|
|
Assignee | |
Comment 4•5 years ago
|
||
Attachment #9080283 -
Attachment is obsolete: true
Attachment #9080283 -
Flags: feedback?(mkmelin+mozilla)
Attachment #9080977 -
Flags: review?(philipp)
|
|
||
Comment 5•5 years ago
|
||
Comment on attachment 9080977 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view.patch
Review of attachment 9080977 [details] [diff] [review]:
-----------------------------------------------------------------
Shouldn't use <table> for all of this. The buttons are not tabular data, but for the rows with title, status etc that's correct. Would use th for the header (title, status, etc.) though.
Attachment #9080977 -
Flags: review?(philipp) → review-
|
|
Assignee | |
Comment 6•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #5)
Shouldn't use <table> for all of this. The buttons are not tabular data, but
for the rows with title, status etc that's correct. Would use th for the
header (title, status, etc.) though.
Here, we need to hide and show the rows. That's why I have used the <table>. With the hbox or html:div way, it will add lots of complexity in JS code for just hiding and showing the rows.
|
|
||
Comment 7•5 years ago
|
||
When do you need to toggle the visibility of the buttons row?
Like I wrote, for title and status etc, it's correct to use table rows
|
|
Assignee | |
Comment 8•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #7)
When do you need to toggle the visibility of the buttons row?
Like I wrote, for title and status etc, it's correct to use table rows
That row also contains the label for priority, which is hidden by default.
|
|
||
Comment 9•5 years ago
|
||
I still think that doesn't sound like tabluar data.
|
|
Assignee | |
Comment 10•5 years ago
|
||
Attachment #9080977 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•5 years ago
|
Attachment #9082044 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 11•5 years ago
|
||
Comment on attachment 9082044 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view.patch
Review of attachment 9082044 [details] [diff] [review]:
-----------------------------------------------------------------
I'm not seeing a priority row even for high priority tasks, on a local calendar, where I'd assume priority is supported. Am I missing something?
::: calendar/base/content/calendar-task-view.js
@@ +125,5 @@
> taskDueRowLabel.style.visibility = taskDueDate ? "visible" : "collapse";
> + if (taskDueDate) {
> + taskDueRowLabel.parentNode.parentNode.removeAttribute("hidden");
> + } else {
> + taskDueRowLabel.parentNode.parentNode.setAttribute("hidden", true);
"hidden" - but then, you can drop this code since there wouldn't be a parentNode, see below
::: calendar/base/content/calendar-task-view.xul
@@ +121,5 @@
> + <html:td>
> + <label id="calendar-task-details-priority-label"
> + value="&calendar.task-details.priority.label;"
> + class="task-details-name"
> + hidden="true"/>
I think the label is not necessary, it can just be in the content of the td.
Well, this is kind of the th for this row, so make it a <th> instead
@@ +141,5 @@
> + value="&calendar.task-details.priority.high.label;"
> + class="task-details-value"
> + crop="end"
> + flex="1"
> + hidden="true"/>
Let's not fix it now, but this is really junk. The value of this cell should be set dynamically instead of having all the content in there and then showing only the relevant part through js. Sigh.
@@ +145,5 @@
> + hidden="true"/>
> + </html:td>
> + </html:tr>
> + <html:tr id="calendar-task-details-title-row"
> + hidden="true">
in html hidden is a boolean attribute, so it exists or it's hidden="hidden" (for true)
@@ +148,5 @@
> + <html:tr id="calendar-task-details-title-row"
> + hidden="true">
> + <html:td>
> + <label value="&calendar.task-details.title.label;"
> + class="task-details-name"/>
don't need the label I think. There's a bunch of that, I won't mention any more occurrences
Attachment #9082044 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 12•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #11)
don't need the label I think. There's a bunch of that, I won't mention any
more occurrences
How can we assign value to <td>/<th>? We have used data-l10n-id at other places in Thunderbird.
|
|
||
Comment 13•5 years ago
|
||
Value? It would be the text content
<html:td class="task-details-name">&calendar.task-details.title.label;</html:td>
|
|
Assignee | |
Comment 14•5 years ago
|
||
Attachment #9082044 -
Attachment is obsolete: true
Attachment #9083305 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 15•5 years ago
|
||
Comment on attachment 9083305 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view.patch
Review of attachment 9083305 [details] [diff] [review]:
-----------------------------------------------------------------
::: calendar/base/content/calendar-task-view.js
@@ +27,4 @@
> return flag;
> }
>
> + function toggleElement(id, flag) {
Please get rid of this helper function, it's not helpful, and I think the name is backwards too.
For each call there is a very standard command to do it, namely toggleAttribute, e.g.
document.getElementById("calendar-task-details-title-row").toggleAttribute("hidden", false);
@@ +59,4 @@
> }
> }
> }
> + if (toggleElement("calendar-task-details-organizer-row", name && name.length)) {
if name is true, name.length will be true
so only check name
@@ +73,5 @@
> displayElement("calendar-task-details-priority-normal", priority == 5);
> displayElement("calendar-task-details-priority-high", priority >= 1 && priority <= 4);
>
> let status = item.getProperty("STATUS");
> + if (toggleElement("calendar-task-details-status-row", status && status.length > 0)) {
just check status, if status is true, status.length is > 0
::: calendar/base/content/calendar-task-view.xul
@@ +146,5 @@
> + </html:tr>
> + <html:tr id="calendar-task-details-title-row"
> + hidden="hidden">
> + <html:td class="task-details-name">
> + &calendar.task-details.title.label;
th?
@@ +149,5 @@
> + <html:td class="task-details-name">
> + &calendar.task-details.title.label;
> + </html:td>
> + <html:td id="calendar-task-details-title"
> + class="task-details-value"/>
td is not self closing in html, so while this works now, please use the closing form
@@ +155,5 @@
> + <html:tr id="calendar-task-details-organizer-row"
> + hidden="hidden">
> + <html:td class="task-details-name">
> + &calendar.task-details.organizer.label;
> + </html:td>
th?
@@ +164,5 @@
> + </html:tr>
> + <html:tr id="calendar-task-details-status-row"
> + hidden="hidden">
> + <html:td class="task-details-name">
> + &calendar.task-details.status.label;
th?
@@ +173,5 @@
> + </html:tr>
> + <html:tr id="calendar-task-details-category-row"
> + hidden="hidden">
> + <html:td class="task-details-name">
> + &calendar.task-details.category.label;
th?
@@ +182,5 @@
> + </html:tr>
> + <html:tr id="task-start-row"
> + class="item-date-row"
> + hidden="hidden">
> + <html:td class="headline">
th?
@@ +193,5 @@
> + <html:tr id="task-due-row"
> + class="item-date-row"
> + hidden="hidden">
> + <html:td class="headline">
> + &calendar.task-details.due.label;
th?
@@ +202,5 @@
> + </html:tr>
> + <html:tr id="calendar-task-details-repeat-row"
> + hidden="hidden">
> + <html:td class="task-details-name">
> + &calendar.task-details.repeat.label;
th?
::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +142,4 @@
> <!ENTITY calendar.task-details.category.label "category">
> <!ENTITY calendar.task-details.repeat.label "repeat">
> <!ENTITY calendar.task-details.attachments.label "attachments">
> +<!ENTITY calendar.task-details.start.label "start date">
can't do it (updated string needs updated localization key)
but you shouldn't do it either. set css white-space: nowrap probably
Attachment #9083305 -
Flags: feedback?(mkmelin+mozilla) → feedback-
|
|
Assignee | |
Comment 16•5 years ago
|
||
Attachment #9083305 -
Attachment is obsolete: true
Attachment #9083671 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 17•5 years ago
|
||
Comment on attachment 9083671 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view.patch
Review of attachment 9083671 [details] [diff] [review]:
-----------------------------------------------------------------
bitrotted quite a bit
::: calendar/base/content/calendar-task-view.xul
@@ +88,5 @@
> + <menuseparator/>
> + </menupopup>
> + </toolbarbutton>
> + <toolbarbutton is="toolbarbutton-menu-button"
> + id="task-actions-markcompleted"
would keep id on the first line
@@ +128,5 @@
> + value="&calendar.task-details.priority.low.label;"
> + class="task-details-value"
> + crop="end"
> + flex="1"
> + hidden="hidden"/>
this is a xul label, not an html one, so it's hidden="true"
Attachment #9083671 -
Flags: feedback?(mkmelin+mozilla)
|
|
Assignee | |
Comment 18•5 years ago
|
||
Attachment #9083671 -
Attachment is obsolete: true
Attachment #9084702 -
Flags: feedback?(mkmelin+mozilla)
|
|
||
Comment 19•5 years ago
|
||
Comment on attachment 9084702 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view-xul.patch
Review of attachment 9084702 [details] [diff] [review]:
-----------------------------------------------------------------
Looks alright to me.
::: calendar/base/content/calendar-task-view.js
@@ +109,5 @@
> }
> }
> let categories = item.getCategories({});
> + if (!document.getElementById("calendar-task-details-category-row")
> + .toggleAttribute("hidden", categories.length <= 0)) {
== 0
Attachment #9084702 -
Flags: feedback?(mkmelin+mozilla) → feedback+
|
|
Assignee | |
Comment 20•5 years ago
|
||
Attachment #9084702 -
Attachment is obsolete: true
Attachment #9085072 -
Flags: review?(paul)
Attachment #9085072 -
Flags: feedback+
|
||
Comment 21•5 years ago
|
||
Comment on attachment 9085072 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view-xul.patch
Review of attachment 9085072 [details] [diff] [review]:
-----------------------------------------------------------------
Looking good. I noticed a few small things to fix up or improve. Will be nice to have these xul grids converted to html.
::: calendar/base/content/calendar-task-view.xul
@@ +115,5 @@
> + </vbox>
> + </hbox>
> + <hbox>
> + <html:table id="calendar-task-details-grid">
> + <html:tr id="calendar-task-details-priority-row"
I think the priority row should go just below the title row, rather than above it. It looks like it wasn't even being shown at all before (at least in TB 60), and it looks weird when the first row is priority.
@@ +141,3 @@
> crop="end"
> flex="1"
> hidden="true"/>
I believe these crop and flex attributes are used for xul but not for html. So we can remove all of these now that we're using html here. Also hidden="hidden" for html here.
@@ +207,5 @@
> + <html:th class="task-details-name">
> + &calendar.task-details.repeat.label;
> + </html:th>
> + <html:td id="calendar-task-details-repeat"
> + crop="end"
Remove 'crop' since we're in html.
::: calendar/base/themes/common/calendar-task-view.css
@@ +28,5 @@
> +}
> +
> +#calendar-task-details-grid > tr > td {
> + padding-inline-start: 6px;
> + font-weight: normal;
I don't think you need the font-weight: normal for the td here. If you drop that here, then you won't need the "!important" for the #calendar-task-details-title font-weight below.
@@ +40,4 @@
> padding-bottom: 0.3em;
> }
>
> #calendar-task-details {
While we're here, can we move this calendar-task-details rule up above the other calendar-task-details-*** rules? Since it is the parent/wrapper element, seems like it should come before the children.
Attachment #9085072 -
Flags: review?(paul) → review-
|
|
Assignee | |
Comment 22•5 years ago
|
||
Attachment #9085072 -
Attachment is obsolete: true
Attachment #9085264 -
Flags: review?(paul)
|
|
||
Comment 23•5 years ago
|
||
Comment on attachment 9085264 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view-xul.patch
Review of attachment 9085264 [details] [diff] [review]:
-----------------------------------------------------------------
Just a few little things remaining and this should be done and ready to go.
::: calendar/base/content/calendar-task-view.xul
@@ +139,1 @@
> hidden="true"/>
hidden="hidden"
@@ +143,1 @@
> hidden="true"/>
hidden="hidden"
@@ +147,1 @@
> hidden="true"/>
hidden="hidden"
::: calendar/base/themes/common/calendar-task-view.css
@@ -121,5 @@
>
> -#calendar-task-details-title {
> - font-weight: bold;
> -}
> -
Oh, I meant that you could drop just the "!important" part of this style (which you had in the previous patch: "font-weight: bold !important"). So don't delete this style but keep it as it is (no "!important" needed) so the title text is still bold.
Attachment #9085264 -
Flags: review?(paul) → review-
|
|
Assignee | |
Comment 24•5 years ago
|
||
Comment on attachment 9085264 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view-xul.patch
Review of attachment 9085264 [details] [diff] [review]:
-----------------------------------------------------------------
::: calendar/base/content/calendar-task-view.xul
@@ +139,1 @@
> hidden="true"/>
This label is XUL element. We generally use html:label with html:input, we don't use html:label to display only text content.
::: calendar/base/themes/common/calendar-task-view.css
@@ +23,5 @@
> +}
> +
> +#calendar-task-details-title {
> + font-weight: bold;
> +}
Added here to keep similar things together.
|
|
||
Comment 25•5 years ago
|
||
Comment on attachment 9085264 [details] [diff] [review]
Bug-1568189_remove-grid-calendar-task-view-xul.patch
r+ Thanks for pointing out what I overlooked.
(In reply to Khushil Mistry [:khushil324] from comment #24)
>
> This label is XUL element. We generally use html:label with html:input, we
> don't use html:label to display only text content.
Ah, right, of course.
> ::: calendar/base/themes/common/calendar-task-view.css
> > +#calendar-task-details-title {
> > + font-weight: bold;
> > +}
>
> Added here to keep similar things together.
Oh, I see it now.
Attachment #9085264 -
Flags: review- → review+
|
|
Assignee | |
Comment 26•5 years ago
|
||
Attachment #9085264 -
Attachment is obsolete: true
Attachment #9085433 -
Flags: review+
|
|
Assignee | |
Updated•5 years ago
|
Keywords: checkin-needed
|
||
Comment 27•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1258058e269a
remove grid usage from calendar-task-view.xul. r=pmorris
|
||
Comment 28•5 years ago
|
||
This is really a Calendar bug, but anyway.
Target Milestone: --- → Thunderbird 70.0
You need to log in
before you can comment on or make changes to this bug.
Description
•