Open
Bug 1391466
Opened 7 years ago
Updated 2 years ago
Integration of react equivalent of calendar-properties-dialog.
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
NEW
People
(Reporter: arshad, Unassigned, Mentored)
Details
Attachments
(2 files, 12 obsolete files)
3.60 KB,
image/png
|
Details | |
112.47 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0 Build ID: 20170612122310
Reporter | ||
Comment 1•7 years ago
|
||
Attachment #8898512 -
Attachment is obsolete: true
Attachment #8898635 -
Flags: feedback?(richard.marti)
Attachment #8898635 -
Flags: feedback?(philipp)
Reporter | ||
Updated•7 years ago
|
Mentor: philipp
Reporter | ||
Updated•7 years ago
|
Summary: Integration of react equivalent of calendar-properties-dialog → Integration of react equivalent of calendar-properties-dialog.
Comment 2•7 years ago
|
||
Comment on attachment 8898635 [details] [diff] [review] react-calendar-properties-dialog.patch Arshad, thank you for this great work! This looks like a native dialog on Windows. Not tested on Linux nor Mac. When I open the dialog it is always shrunken. One click on the bottom or top border or on a corner expands it immediately to the correct size. This needs to be fixed before landing. The CSS seems to need some cleanup. For example the input[type="text"] has in global.css line 69 an !important padding and then in line 82 as .row > input[type="text"].row-input again a normal padding which is never used because of the !important of line 69. This makes this rule obsolete. This is a minor issue and can be cleaned up later. All in all, this looks very promising. :)
Attachment #8898635 -
Flags: feedback?(richard.marti) → feedback+
Comment 3•7 years ago
|
||
This screenshot shows how the dialog looks when opening. It's not only on initial opening, it opens always shrunken.
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #3) > Created attachment 8898676 [details] > Dialog shrunken on opening > > This screenshot shows how the dialog looks when opening. It's not only on > initial opening, it opens always shrunken. I knew that this issue will arise. In dialogs, `window.sizeToContent` was used to size the window height, but when an react iframe is loaded then window isn't able to adjust its height according to iframe content height. I have added a js hack which works perfect on my pc(linux). I think just making a minor tweak in a timeout will remove this issue.
Reporter | ||
Comment 5•7 years ago
|
||
Removed the overriding css and modified code for setting window height. Hopefully this will remove the shrunken window issue reported by Richard Marti.
Attachment #8898736 -
Flags: feedback?(richard.marti)
Attachment #8898736 -
Flags: feedback?(philipp)
Comment 6•7 years ago
|
||
When you send the next patch, can you please remove all trailing spaces you've introduced. You can see them in pink in the review (Splinter Review). Also, please supersede previous patches.
Comment 7•7 years ago
|
||
Comment on attachment 8898736 [details] [diff] [review] react-calendar-properties-dialog.patch The dialog is still shrunken. :(
Attachment #8898736 -
Flags: feedback?(richard.marti)
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #6) > When you send the next patch, can you please remove all trailing spaces > you've introduced. You can see them in pink in the review (Splinter Review). > Also, please supersede previous patches. Ok, I will do that.
Reporter | ||
Comment 9•7 years ago
|
||
Removed the shrunken window and overriding css issue reported by Richard Marti.
Attachment #8898635 -
Attachment is obsolete: true
Attachment #8898736 -
Attachment is obsolete: true
Attachment #8898635 -
Flags: feedback?(philipp)
Attachment #8898736 -
Flags: feedback?(philipp)
Attachment #8899206 -
Flags: feedback?(richard.marti)
Attachment #8899206 -
Flags: feedback?(philipp)
Comment 10•7 years ago
|
||
Comment on attachment 8899206 [details] [diff] [review] react-calendar-properties-dialog.patch A lot better as the dialog has at the end the correct height. But I see a jumping from the initial height to the final one during opening, also on Mac. It looks like a stuttering of the opening animation. Maybe Philipp has a solution how this could be fixed.
Attachment #8899206 -
Flags: feedback?(richard.marti) → feedback+
Reporter | ||
Comment 11•7 years ago
|
||
Removed all XUL from calendar properties dialog.
Attachment #8899206 -
Attachment is obsolete: true
Attachment #8899206 -
Flags: feedback?(philipp)
Attachment #8899501 -
Flags: feedback?(richard.marti)
Attachment #8899501 -
Flags: feedback?(philipp)
Comment 12•7 years ago
|
||
Comment on attachment 8899501 [details] [diff] [review] react-calendar-properties-dialog(html).patch The patch patch doesn't apply. It fails with a undefined error. Please can you apply a new patch?
Attachment #8899501 -
Flags: feedback?(richard.marti)
Reporter | ||
Comment 13•7 years ago
|
||
This patch also closes Bug 1151440 [1] [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1151440
Attachment #8899501 -
Attachment is obsolete: true
Attachment #8899501 -
Flags: feedback?(philipp)
Attachment #8899579 -
Flags: feedback?(richard.marti)
Attachment #8899579 -
Flags: feedback?(philipp)
Comment 14•7 years ago
|
||
Comment on attachment 8899579 [details] [diff] [review] react-prop-dialog-html.patch Thanks, looks great. The window opens now as it should like without patch. But I see two things: - First, the dialog opens always in the center of the screen. Is it possible to give the dialog a persistent position like the XUL dialogs can? - When a longer text is shown in the dialog, like the force-disabled-description text, then the dialog grows to the width of the text. With the English text it's not so bad but with other locales this can be very longer. So you should give also a max-width.
Attachment #8899579 -
Flags: feedback?(richard.marti) → feedback+
Reporter | ||
Comment 15•7 years ago
|
||
Changes in patch: - Set a max width for force-disabled text
Attachment #8899579 -
Attachment is obsolete: true
Attachment #8899579 -
Flags: feedback?(philipp)
Reporter | ||
Comment 16•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #14) > Comment on attachment 8899579 [details] [diff] [review] > react-prop-dialog-html.patch > > Thanks, looks great. The window opens now as it should like without patch. > > But I see two things: > - First, the dialog opens always in the center of the screen. Is it possible > to give the dialog a persistent position like the XUL dialogs can? > - When a longer text is shown in the dialog, like the > force-disabled-description text, then the dialog grows to the width of the > text. With the English text it's not so bad but with other locales this can > be very longer. So you should give also a max-width. I dont think there is any HTML feature to make persistent positioned popups/windows. It is possible but I am little bit short on time as gsoc is near its end. If it is important then I will try to add it as soon as I am done with rest of dialogs.
Reporter | ||
Comment 17•7 years ago
|
||
refreshInterval state now get null as value when calendar can't be refresh. Earlier it was getting undefined.
Attachment #8899810 -
Attachment is obsolete: true
Reporter | ||
Comment 18•7 years ago
|
||
- added mozilla license comments to all files - tab size corrected for css/html files - persistent screenX, screenY - persistent height and width of dialog
Attachment #8900031 -
Attachment is obsolete: true
Attachment #8902538 -
Flags: feedback?(richard.marti)
Attachment #8902538 -
Flags: feedback?(philipp)
Comment 19•7 years ago
|
||
Comment on attachment 8902538 [details] [diff] [review] react-calendar-properties-dialog.patch The patch fails on applying. Please can you attach a new one?
Attachment #8902538 -
Flags: feedback?(richard.marti)
Reporter | ||
Comment 20•7 years ago
|
||
Attachment #8902538 -
Attachment is obsolete: true
Attachment #8902538 -
Flags: feedback?(philipp)
Attachment #8903074 -
Flags: feedback?(richard.marti)
Attachment #8903074 -
Flags: feedback?(philipp)
Comment 21•7 years ago
|
||
Comment on attachment 8903074 [details] [diff] [review] calpropdialog.patch Looks good, thanks. What should be added is the dialog max-width I mentioned in comment 14.
Attachment #8903074 -
Flags: feedback?(richard.marti) → feedback+
Reporter | ||
Comment 22•7 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #21) > Comment on attachment 8903074 [details] [diff] [review] > calpropdialog.patch > > Looks good, thanks. What should be added is the dialog max-width I mentioned > in comment 14. I had add max-width for the force-disabled text only. https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1391466&attachment=8903074
Reporter | ||
Comment 23•7 years ago
|
||
Comment on attachment 8903074 [details] [diff] [review] calpropdialog.patch Review of attachment 8903074 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/themes/common/dialogs/calendar-properties-dialog.css @@ +35,5 @@ > +} > + > +p#force-disabled-description { > + margin: 0 0 10px; > + max-width: 700px; max-width set as per comment 14.
Reporter | ||
Comment 24•7 years ago
|
||
Added licence comment to react-utils.js file.
Attachment #8903074 -
Attachment is obsolete: true
Attachment #8903074 -
Flags: feedback?(philipp)
Attachment #8917883 -
Flags: review?(richard.marti)
Attachment #8917883 -
Flags: review?(philipp)
Comment 25•7 years ago
|
||
Comment on attachment 8917883 [details] [diff] [review] cal-prop-dialog.patch Patch doesn't apply. Please could you split the patch into a cal-prop patch and a common-files (CSS etc.) patch? Like this, applying this patch and the one from bug 1393694 together could work. Also at check-in this will make much trouble. A short check of some common files in both patches showed also some small differences in sorting of rules and white spaces. Using one patch for common files would it also make easier to review as it needs then only to be done one time.
Attachment #8917883 -
Flags: review?(richard.marti)
Reporter | ||
Comment 26•7 years ago
|
||
Ok, I will try to do this asap!
Updated•7 years ago
|
Component: Untriaged → General
Product: Thunderbird → Calendar
Version: 57 Branch → unspecified
Reporter | ||
Comment 27•6 years ago
|
||
This patch is an old patch uploaded before the latest patch which fails on applying. Common files aren't separated into separate patch as doing that at this stage is very difficult task.
Attachment #8917883 -
Attachment is obsolete: true
Attachment #8917883 -
Flags: review?(philipp)
Comment 28•6 years ago
|
||
Philipp, are we going to proceed with this bug (and bug 1393694), or was this a "proof of concept" only?
Flags: needinfo?(philipp)
Comment 29•6 years ago
|
||
We're going to proceed. Ball is in my court to go through the patch and make it ready to land, I'm really trying to get to it this weekend.
Updated•6 years ago
|
Assignee: nobody → arshdkhn1
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(philipp)
Comment 30•6 years ago
|
||
So this is just the unbitrotted patch. I've discussed with Arshad about the common css files and he will follow up.
Attachment #8940791 -
Attachment is obsolete: true
Comment 31•6 years ago
|
||
Drive-by comment. In the previous patch calendar-properties-dialog.js and calendar-properties-dialog.xul were removed. In the new patch, the XUL file is not touched and calendar-properties-dialog.js is moved and then has its content almost completely replaced. IMHO the previous approach was better: Remove the file and put a new one in its place in a different directory. A moved file with 100% differences doesn't make much sense.
Comment 32•6 years ago
|
||
Sorry, you moved the XUL file to .html. That doesn't convince me.
Comment 33•6 years ago
|
||
It helps see the progression of the dialogs. I think this is valuable even if the content is different. Thanks for the comment though!
Comment 34•6 years ago
|
||
Hmm, "It helps see the progression of the dialogs" - how exactly? You read the changeset? You can also see the "out with the old - in with the new". In the IMAP/News subscribe dialogue we tossed the mail/ and suite/ files completely and put a new one into mailnews/ since the content was about 99% different. Anyway, maybe a matter of taste. So just ignore me.
Reporter | ||
Updated•5 years ago
|
Assignee: arshdkhn1 → nobody
Status: ASSIGNED → NEW
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•