1391466 - Integration of react equivalent of calendar-properties-dialog.

Status: NEW

(Calendar :: General, defect)

Description

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog.patch

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170612122310

Comment 1

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog.patch

Comment 2

[Richard Marti (:Paenglab)]

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. :)

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Dialog shrunken on opening

This screenshot shows how the dialog looks when opening. It's not only on initial opening, it opens always shrunken.

Comment 4

[Arshad Khan [:arshad]]

(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.

Comment 5

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog.patch

Removed the overriding css and modified code for setting window height.
Hopefully this will remove the shrunken window issue reported by Richard Marti.

Comment 6

[Jorg K (CEST = GMT+2)]

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

[Richard Marti (:Paenglab)]

Comment on attachment 8898736 [details] [diff] [review]
react-calendar-properties-dialog.patch

The dialog is still shrunken. :(

Comment 8

[Arshad Khan [:arshad]]

(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.

Comment 9

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog.patch

Removed the shrunken window and overriding css issue reported by Richard Marti.

Comment 10

[Richard Marti (:Paenglab)]

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.

Comment 11

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog(html).patch

Removed all XUL from calendar properties dialog.

Comment 12

[Richard Marti (:Paenglab)]

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?

Comment 13

[Arshad Khan [:arshad]]

Attachment: react-prop-dialog-html.patch

This patch also closes Bug 1151440 [1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1151440

Comment 14

[Richard Marti (:Paenglab)]

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.

Comment 15

[Arshad Khan [:arshad]]

Attachment: react-cal-prop-dialog-html.patch

Changes in patch:
- Set a max width for force-disabled text

Comment 16

[Arshad Khan [:arshad]]

(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.

Comment 17

[Arshad Khan [:arshad]]

Attachment: react-cal-prop-dialog.patch

refreshInterval state now get null as value when calendar can't be refresh. Earlier it was getting undefined.

Comment 18

[Arshad Khan [:arshad]]

Attachment: react-calendar-properties-dialog.patch

- added mozilla license comments to all files
- tab size corrected for css/html files
- persistent screenX, screenY
- persistent height and width of dialog

Comment 19

[Richard Marti (:Paenglab)]

Comment on attachment 8902538 [details] [diff] [review]
react-calendar-properties-dialog.patch

The patch fails on applying. Please can you attach a new one?

Comment 20

[Arshad Khan [:arshad]]

Attachment: calpropdialog.patch

Comment 21

[Richard Marti (:Paenglab)]

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.

Comment 22

[Arshad Khan [:arshad]]

(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

Comment 23

[Arshad Khan [:arshad]]

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.

Comment 24

[Arshad Khan [:arshad]]

Attachment: cal-prop-dialog.patch

Added licence comment to react-utils.js file.

Comment 25

[Richard Marti (:Paenglab)]

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.

Comment 26

[Arshad Khan [:arshad]]

Ok, I will try to do this asap!

Comment 27

[Arshad Khan [:arshad]]

Attachment: calendar-property-dialog.patch

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.

Comment 28

[Jorg K (CEST = GMT+2)]

Philipp, are we going to proceed with this bug (and bug 1393694), or was this a "proof of concept" only?

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v2

So this is just the unbitrotted patch. I've discussed with Arshad about the common css files and he will follow up.

Comment 31

[Jorg K (CEST = GMT+2)]

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

[Jorg K (CEST = GMT+2)]

Sorry, you moved the XUL file to .html. That doesn't convince me.

Comment 33

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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

[Jorg K (CEST = GMT+2)]

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.