Integration of react equivalent of calendar-properties-dialog.

ASSIGNED
Assigned to

Status

ASSIGNED
2 years ago
8 months ago

People

(Reporter: arshad, Assigned: arshad, Mentored)

Tracking

Details

Attachments

(2 attachments, 12 obsolete attachments)

3.60 KB, image/png
Details
112.47 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
Posted patch react-calendar-properties-dialog.patch (obsolete) β€” β€” Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170612122310
(Assignee)

Comment 1

2 years ago
Posted patch react-calendar-properties-dialog.patch (obsolete) β€” β€” Splinter Review
Attachment #8898512 - Attachment is obsolete: true
Attachment #8898635 - Flags: feedback?(richard.marti)
Attachment #8898635 - Flags: feedback?(philipp)
(Assignee)

Updated

2 years ago
Mentor: philipp
(Assignee)

Updated

2 years ago
Summary: Integration of react equivalent of calendar-properties-dialog → Integration of react equivalent of calendar-properties-dialog.
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+
This screenshot shows how the dialog looks when opening. It's not only on initial opening, it opens always shrunken.
(Assignee)

Comment 4

2 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.
(Assignee)

Comment 5

2 years ago
Posted patch react-calendar-properties-dialog.patch (obsolete) β€” β€” Splinter Review
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

2 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 on attachment 8898736 [details] [diff] [review]
react-calendar-properties-dialog.patch

The dialog is still shrunken. :(
Attachment #8898736 - Flags: feedback?(richard.marti)
(Assignee)

Comment 8

2 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.
(Assignee)

Comment 9

2 years ago
Posted patch react-calendar-properties-dialog.patch (obsolete) β€” β€” Splinter Review
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 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+
(Assignee)

Comment 11

2 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 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)
(Assignee)

Comment 13

2 years ago
Posted patch react-prop-dialog-html.patch (obsolete) β€” β€” Splinter Review
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 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+
(Assignee)

Comment 15

2 years ago
Posted patch react-cal-prop-dialog-html.patch (obsolete) β€” β€” Splinter Review
Changes in patch:
- Set a max width for force-disabled text
Attachment #8899579 - Attachment is obsolete: true
Attachment #8899579 - Flags: feedback?(philipp)
(Assignee)

Comment 16

2 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.
(Assignee)

Comment 17

2 years ago
Posted patch react-cal-prop-dialog.patch (obsolete) β€” β€” Splinter Review
refreshInterval state now get null as value when calendar can't be refresh. Earlier it was getting undefined.
Attachment #8899810 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Posted patch react-calendar-properties-dialog.patch (obsolete) β€” β€” Splinter Review
- 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 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)
(Assignee)

Comment 20

2 years ago
Posted patch calpropdialog.patch (obsolete) β€” β€” Splinter Review
Attachment #8902538 - Attachment is obsolete: true
Attachment #8902538 - Flags: feedback?(philipp)
Attachment #8903074 - Flags: feedback?(richard.marti)
Attachment #8903074 - Flags: feedback?(philipp)
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+
(Assignee)

Comment 22

2 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
(Assignee)

Comment 23

2 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.
(Assignee)

Comment 24

2 years ago
Posted patch cal-prop-dialog.patch (obsolete) β€” β€” Splinter Review
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 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)
(Assignee)

Comment 26

2 years ago
Ok, I will try to do this asap!
Component: Untriaged → General
Product: Thunderbird → Calendar
Version: 57 Branch → unspecified
(Assignee)

Comment 27

a year ago
Posted patch calendar-property-dialog.patch (obsolete) β€” β€” Splinter Review
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

a year ago
Philipp, are we going to proceed with this bug (and bug 1393694), or was this a "proof of concept" only?
Flags: needinfo?(philipp)
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.
Assignee: nobody → arshdkhn1
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(philipp)
Posted patch Fix - v2 β€” β€” Splinter Review
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

a year 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

a year ago
Sorry, you moved the XUL file to .html. That doesn't convince me.
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

a year 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.
You need to log in before you can comment on or make changes to this bug.