Closed Bug 1568224 Opened 4 months ago Closed 3 months ago

remove grid usage from comm/calendar/base/content/dialogs/calendar-error-prompt.xul

Categories

(Thunderbird :: General, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: khushil324, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → khushil324
Attachment #9080025 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9080025 [details] [diff] [review]
Bug-1568224_remove-grid-calendar-error-prompt.patch

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

I don't think this should be <table>, as it's not really tabular data. The actual layout is also fairly ugly from start...
I'd suggest to use normal hbox/vbox and not have the description textbox next to the label, but under it, like

Error code: 123
Description:
| Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Excepteur sint occaecat cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id est laborum.
|  Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. Duis aute irure dolor in reprehenderit in voluptate velit esse cillum dolore eu fugiat nulla pariatur.
|
Attachment #9080025 - Flags: feedback?(mkmelin+mozilla)
Attachment #9080025 - Attachment is obsolete: true
Attachment #9085109 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9085109 [details] [diff] [review]
Bug-1568224_remove-grid-calendar-error-prompt-xul.patch

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

::: calendar/base/content/dialogs/calendar-error-prompt.xul
@@ -26,3 @@
>      <script src="chrome://global/content/editMenuOverlay.js"/>
>      <script><![CDATA[
>          function loadErrorPrompt() {

please move the js into an calendar-error-prompt.js file while you're here.

@@ +32,5 @@
>              document.getElementById("error-description").value = args.GetString(2);
>              this.sizeToContent();
>          }
>          function toggleDetails() {
> +            let grid = document.getElementById("details-box");

please name the variable details. It's not a grid anymore

@@ +56,5 @@
> +            </hbox>
> +            <vbox>
> +                <label value="&calendar.error.description;" control="error-description"/>
> +                <html:textarea id="error-description" readonly="readonly" rows="5"
> +                               style="margin-inline-start:6px; border:none;"/>

would avoid inline stylings, but they do not seem necessary
Attachment #9085109 - Flags: feedback?(mkmelin+mozilla) → feedback+
Attachment #9085109 - Attachment is obsolete: true
Attachment #9085379 - Flags: review?(paul)
Attachment #9085379 - Flags: feedback+
Comment on attachment 9085379 [details] [diff] [review]
Bug-1568224_remove-grid-calendar-error-prompt-xul.patch

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

Looks good overall, but I have a question about CSS styles, so r+ with that fixed.

::: calendar/base/content/dialogs/calendar-error-prompt.xul
@@ +38,5 @@
> +                <label id="error-code" value=""/>
> +            </hbox>
> +            <vbox>
> +                <label value="&calendar.error.description;" control="error-description"/>
> +                <html:textarea id="error-description" class="plain" readonly="readonly" rows="5"/>

I see you've backed out the inline stylings that were in the previous patch, but I don't see any styles for this "plain" class in the css file(s).  Am I just missing them?  If not, then it seems we should either go back to the inline styles or add a stylesheet that connects them to this 'plain' class.
Attachment #9085379 - Flags: review?(paul) → review+

(In reply to Paul Morris [:pmorris] from comment #6)

I see you've backed out the inline stylings that were in the previous patch,
but I don't see any styles for this "plain" class in the css file(s). Am I
just missing them? If not, then it seems we should either go back to the
inline styles or add a stylesheet that connects them to this 'plain' class.

It is applying the following CSS on Mac:
-moz-appearance: none;
margin: 0 !important;
border: none;
padding: 0;

and it is coming from global.css.

(In reply to Richard Marti (:Paenglab) from comment #8)

.plain is in global.css, see https://searchfox.org/mozilla-central/source/toolkit/themes/shared/global.inc.css#7

Ah, I see it now, thanks. I was looking in "global.css" but not "global.inc.css". This is good to go.

Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f354e2beb494
remove grid usage from calendar-error-prompt.xul. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
You need to log in before you can comment on or make changes to this bug.