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)

Product:
Thunderbird
Component:
General
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 70.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 9 obsolete files)

Bug-1568189_remove-grid-calendar-task-view-xul.patch
29.00 KB, patch
khushil324
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → khushil324
Attachment #9079991 - Flags: feedback?(mkmelin+mozilla)
Blocks: tb-burn-xul-grids
Status: NEW → ASSIGNED
Attachment #9079991 - Attachment is obsolete: true
Attachment #9079991 - Flags: feedback?(mkmelin+mozilla)
Attachment #9080283 - Flags: feedback?(mkmelin+mozilla)
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
Attachment #9080283 - Attachment is obsolete: true
Attachment #9080283 - Flags: feedback?(mkmelin+mozilla)
Attachment #9080977 - Flags: review?(philipp)
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-

(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.

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

(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.

I still think that doesn't sound like tabluar data.

Attachment #9080977 - Attachment is obsolete: true
Attachment #9082044 - Flags: feedback?(mkmelin+mozilla)
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)

(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.

Value? It would be the text content

<html:td class="task-details-name">&calendar.task-details.title.label;</html:td>

Attachment #9082044 - Attachment is obsolete: true
Attachment #9083305 - Flags: feedback?(mkmelin+mozilla)
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&#160;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-
Attachment #9083305 - Attachment is obsolete: true
Attachment #9083671 - Flags: feedback?(mkmelin+mozilla)
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)
Attachment #9083671 - Attachment is obsolete: true
Attachment #9084702 - Flags: feedback?(mkmelin+mozilla)
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+
Attachment #9084702 - Attachment is obsolete: true
Attachment #9085072 - Flags: review?(paul)
Attachment #9085072 - Flags: feedback+
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-
Attachment #9085072 - Attachment is obsolete: true
Attachment #9085264 - Flags: review?(paul)
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-
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 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+
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1258058e269a
remove grid usage from calendar-task-view.xul. r=pmorris

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

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.

Attachment

General

Creator:
Created:
Updated:
Size: