remove grid usage from comm/mail/components/compose/content/dialogs/EdDictionary.xul
Categories
(Thunderbird :: Message Compose Window, task)
Tracking
(Not tracked)
People
(Reporter: khushil324, Assigned: khushil324)
References
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Comment on attachment 9092354 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9092354 [details] [diff] [review]: ----------------------------------------------------------------- The Add button is now thinner than it used to be. Maybe the reason why the Remove and Replace buttons are not aligned like they use to (now higher up than they used to) The "Replace" functionality is really odd. I think we can remove that. In the odd case people go into this dialog, they can add the new and delete the old. That the currently selected word is replaced with the one in "new word", well that's unexpected. ::: mail/components/compose/content/dialogs/EdDictionary.xul @@ +30,5 @@ > + flex="1" > + height="150px"/> > + </vbox> > + <vbox flex="1"> > + <label value=""/> I'm not sure why this is there, but it's a hack at best. Can we remove these? @@ +38,5 @@ > + <vbox flex="1"> > + <button id="ReplaceWord" oncommand="ReplaceWord()" label="&ReplaceButton.label;" > + accesskey="&ReplaceButton.accessKey;"/> > + <button id="RemoveWord" oncommand="RemoveWord()" label="&RemoveButton.label;" > + accesskey="&RemoveButton.accessKey;"/> please fix the double spaces
Comment 4•5 years ago
|
||
Comment on attachment 9092354 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9092354 [details] [diff] [review]: ----------------------------------------------------------------- The Add button is now thinner than it used to be. Maybe the reason why the Remove and Replace buttons are not aligned like they use to (now higher up than they used to) The "Replace" functionality is really odd. I think we can remove that. In the odd case people go into this dialog, they can add the new and delete the old. That the currently selected word is replaced with the one in "new word", well that's unexpected. ::: mail/components/compose/content/dialogs/EdDictionary.xul @@ +30,5 @@ > + flex="1" > + height="150px"/> > + </vbox> > + <vbox flex="1"> > + <label value=""/> I'm not sure why this is there, but it's a hack at best. Can we remove these? @@ +38,5 @@ > + <vbox flex="1"> > + <button id="ReplaceWord" oncommand="ReplaceWord()" label="&ReplaceButton.label;" > + accesskey="&ReplaceButton.accessKey;"/> > + <button id="RemoveWord" oncommand="RemoveWord()" label="&RemoveButton.label;" > + accesskey="&RemoveButton.accessKey;"/> please fix the double spaces
Assignee | ||
Comment 5•5 years ago
|
||
For Linux, on the trunk, the add button is little thicker and looking little odd. It was thicker than all the other buttons. Now, all the buttons are of the same thickness.
Comment 6•5 years ago
|
||
Comment on attachment 9092739 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9092739 [details] [diff] [review]: ----------------------------------------------------------------- Better, but top of the Remove button is still not aligned with the top of the word box (a few pixels off)
Assignee | ||
Comment 7•5 years ago
|
||
Comment 8•5 years ago
|
||
Comment on attachment 9092742 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9092742 [details] [diff] [review]: ----------------------------------------------------------------- Sill not completely right, will attach a screenshot. But, looking at the code more, it seems to me this isn't a very good way to convert it. Using flexbox would seem like a more sane structure. Also, the close button placement is a bit odd. (Usually, it's separate, not in connection with the list area.)
Assignee | ||
Comment 9•5 years ago
|
||
Comment 10•5 years ago
|
||
Comment on attachment 9092860 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9092860 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/dialogs/EdDictionary.xul @@ +42,5 @@ > + control="DictionaryList" > + accesskey="&DictionaryList.accessKey;"/> > + </div> > + <div> > + </div> please remove the empty divs. When you add those, you're doing something wrong. Should be able to add a rule like below, and add that instead .grid-item-span-row { grid-column: 1 / -1; }
Assignee | ||
Comment 11•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
Comment on attachment 9093242 [details] [diff] [review] Bug-1580715_remove-grid-EdDictionary.patch Review of attachment 9093242 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=mkmelin Unrelated to this bug, when opening this dialog I get JavaScript error: chrome://messenger/content/messengercompose/EdSpellCheck.js, line 210: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord] ... and closing it I get JavaScript error: chrome://messenger/content/messengercompose/EdDictionary.xul, line 1: ReferenceError: onClose is not defined Could you please file bugs. For the latter, should be related to bug 1541789. https://hg.mozilla.org/comm-central/log/tip/editor/ui/dialogs/content/EdDictionary.js
Assignee | ||
Comment 14•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #13)
JavaScript error:
chrome://messenger/content/messengercompose/EdSpellCheck.js, line 210:
NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001
(NS_ERROR_NOT_INITIALIZED) [nsIEditorSpellCheck.GetNextMisspelledWord]
This error is shown when editor part is not selected and you click on the Spelling button to check the spelling. I will open the new bug for this.
... and closing it I get
JavaScript error:
chrome://messenger/content/messengercompose/EdDictionary.xul, line 1:
ReferenceError: onClose is not defined
This is resolved with this patch as I have added onclose function in the dialog.
Assignee | ||
Updated•5 years ago
|
Comment 15•5 years ago
|
||
You added the onclose, but not the function?
https://hg.mozilla.org/comm-central/diff/76cba5d0233aded3bbb881c0e6440a0a9f2015ab/editor/ui/dialogs/content/EdDictionary.js
Assignee | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
Then we need to integrate it with the grid-layout. Can we change the label of the cancel button?
Comment 19•5 years ago
|
||
I think you can set buttonlabelaccept="&CloseButton.label;" (and buttons="accept")
Assignee | ||
Comment 20•5 years ago
|
||
Assignee | ||
Comment 21•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 22•5 years ago
|
||
can you please do the commit messages like so:
Bug 1580715 - remove grid usage from EdDictionary.xul. r=mkmelin
That's the standard suggested here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch
I know, it's just a small detail, but I prefer everyone to used the same punctuation rules.
Comment 23•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0e468cd9a91b
remove grid usage from EdDictionary.xul, remove Replace button. r=mkmelin
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
(In reply to Jorg K (GMT+2) (reduced availability 14-19 of Sept.) from comment #22)
can you please do the commit messages like so:
Bug 1580715 - remove grid usage from EdDictionary.xul. r=mkmelin
That's the standard suggested here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patchI know, it's just a small detail, but I prefer everyone to used the same punctuation rules.
Sure, I will keep this in mind now onwards.
Description
•