Closed Bug 1534249 Opened 6 years ago Closed 5 years ago

remove grid usage from comm/mail/components/im/content/addbuddy.xul, comm/mail/components/im/content/imAccountWizard.xul and comm/mail/components/im/content/joinchat.xul

Categories

(Thunderbird :: Instant Messaging, task)

Product:
Thunderbird
Component:
Instant Messaging
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0

People

(Reporter: khushil324, Assigned: khushil324)

References

Details

Attachments

(1 file, 8 obsolete files)

Bug-1534249_remove-grid-chat.patch
(deleted), patch
khushil324
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → khushil324
Blocks: tb-burn-xul-grids
Status: NEW → ASSIGNED
Attached patch remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9051106 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9051106 [details] [diff] [review] remove_grid_chat.patch Too bitrotted to review. Please rebase
Attachment #9051106 - Attachment is obsolete: true
Attachment #9051106 - Flags: review?(mkmelin+mozilla)
Attached patch remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9051688 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9051688 [details] [diff] [review] remove_grid_chat.patch Review of attachment 9051688 [details] [diff] [review]: ----------------------------------------------------------------- Please fix the comment message format "Bug xxx - ..." There's some bitrot, but seems to be working ::: mail/components/im/content/joinchat.xul @@ +23,5 @@ > + <hbox flex="1" align="center"> > + <label value="&account.label;" control="accountlist"/> > + </hbox> > + <separator class="thin" id="separatorCol1Start"/> > + <separator class="thin" id="separatorCol1End"/> wouldn't it be better to have an hbox for these and clear that when needed? @@ +32,5 @@ > + <vbox> > + <menulist id="accountlist" onselect="joinChat.onAccountSelect();"/> > + <separator class="thin" id="separatorCol2Start"/> > + <separator class="thin" id="separatorCol2End"/> > + <checkbox label="&autojoin.label;" accesskey="&autojoin.accesskey;" id="autojoin"/> move ids first @@ +39,5 @@ > + <hbox flex="1"> > + <label value=""/> > + </hbox> > + <separator class="thin" id="separatorCol3Start"/> > + <separator class="thin" id="separatorCol3End"/> same thing here
Attachment #9051688 - Flags: review?(mkmelin+mozilla)
Attached patch Bug-1534249_remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9051688 - Attachment is obsolete: true
Attachment #9057114 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9057114 [details] [diff] [review] Bug-1534249_remove_grid_chat.patch Review of attachment 9057114 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/im/content/joinchat.xul @@ +31,5 @@ > + </vbox> > + <vbox> > + <menulist id="accountlist" onselect="joinChat.onAccountSelect();"/> > + <hbox id="separatorCol2Start" class="thin"/> > + <hbox id="separatorCol2End" class="thin"/> hmm? looks odd, and the things you do in javascript should probably be adjusted.
Attachment #9057114 - Flags: review?(mkmelin+mozilla)

hmm? looks odd, and the things you do in javascript should probably be
adjusted.

We are using hbox/separator as reference points where we want dynamic XUL elements(i.e. elements we are inserting through javascript) in between. We just need their ids for querying. So anything will work, hbox or separator.

Yes but you seem to iterate until getting to the next "separator" which is of course not relevant if all you need is to clean the hbox children

We are not inserting anything as a child of separator/hbox. We are inserting hbox with a child element of label as a next sibling of separator/hbox. And we are removing all the elements at the start with an iteration so that we can again insert hbox with label child element according to relevant service.
Do you want to get rid of separator logic and want all the new inserted element as a child of separator/hbox/vbox? But there, flex property will create a problem.

If it's possible, that would be more logical. The current approach is a bit of a hack.

Attached patch Bug-1534249_remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9057114 - Attachment is obsolete: true
Attachment #9057399 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9057399 [details] [diff] [review] Bug-1534249_remove_grid_chat.patch Review of attachment 9057399 [details] [diff] [review]: ----------------------------------------------------------------- Not too happy about joinchat.xul - that's really quite regarding markup. Could you check just using css grid layout and make it more reasonable. Think about how you would do this in html instead. (In fact we could consider converting this window to that.) ::: mail/components/im/content/imAccountWizard.xul @@ +82,4 @@ > onkeypress="accountWizard.onGroupboxKeypress(event)"> > <caption id="protoSpecificCaption" > onclick="accountWizard.toggleGroupbox('protoSpecificGroupbox')"/> > + <vbox flex="1" id="protoSpecific"/> move id first ::: mail/components/im/content/joinchat.js @@ +99,3 @@ > } > + hboxOption.appendChild(optionLabel); > + col3.appendChild(hboxOption); I think instead of setting the text dynamically, you should just collapse col3 when needed ::: mail/components/im/content/joinchat.xul @@ +30,5 @@ > + </vbox> > + <vbox> > + <menulist id="accountlist" onselect="joinChat.onAccountSelect();"/> > + <vbox id="separatorCol2"/> > + <checkbox label="&autojoin.label;" accesskey="&autojoin.accesskey;" id="autojoin"/> while you're here, move id first
Attachment #9057399 - Flags: review?(mkmelin+mozilla)

That was supposed to say "really quite a mess..."

Attached patch Bug-1534249_remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9057399 - Attachment is obsolete: true
Attachment #9058191 - Flags: review?(mkmelin+mozilla)
Attached patch Bug-1534249_remove_grid_chat.patch (obsolete) (deleted) — — Splinter Review

I think the joinchat changes were not quite correct. Check this out and see what you think.

The auto-join checkbox is now in another place, but I think it was misplaced to begin with.

Attachment #9058191 - Attachment is obsolete: true
Attachment #9058191 - Flags: review?(mkmelin+mozilla)
Attachment #9058251 - Flags: review?(khushil324)
Attached patch Bug-1534249_remove-grid-chat.patch (obsolete) (deleted) — — Splinter Review

It worked, Thanks for the help. Now, I am also adding Autojoin Checkbox through Javascript.

Attachment #9058251 - Attachment is obsolete: true
Attachment #9058251 - Flags: review?(khushil324)
Attachment #9058480 - Flags: review?(mkmelin+mozilla)

But why add it through script? For the placement, I think having it under the middle column is actually a bit strange

Attached patch Bug-1534249_remove-grid-chat.patch (obsolete) (deleted) — — Splinter Review
Attachment #9058480 - Attachment is obsolete: true
Attachment #9058480 - Flags: review?(mkmelin+mozilla)
Attachment #9058880 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9058880 [details] [diff] [review] Bug-1534249_remove-grid-chat.patch Review of attachment 9058880 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin ::: mail/components/im/content/joinchat.js @@ +78,4 @@ > if (!field.required) { > + div3.classList.remove("required"); > + } else { > + div3.classList.add("required"); I just realized this all could be written simpler as div3.classList.toggle("required", field.required);
Attachment #9058880 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9058880 - Attachment is obsolete: true
Attachment #9060897 - Flags: review+
Type: enhancement → defect
Keywords: checkin-needed

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/56e61fde5f62
remove grid usage from addbuddy.xul, imAccountWizard.xul and joinchat.xul. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 68.0
Type: defect → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: