Last Comment Bug 1391466 - Integration of react equivalent of calendar-properties-dialog.
: Integration of react equivalent of calendar-properties-dialog.
Status: ASSIGNED
:
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: ---
Assigned To: Arshad Khan [:arshad]
:
:
Mentors: Philipp Kewisch [:Fallen] [:📆]
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-17 16:28 PDT by Arshad Khan [:arshad]
Modified: 2018-08-03 05:45 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
react-calendar-properties-dialog.patch (102.70 KB, patch)
2017-08-17 16:28 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
react-calendar-properties-dialog.patch (102.72 KB, patch)
2017-08-17 23:06 PDT, Arshad Khan [:arshad]
Paenglab: feedback+
Details | Diff | Splinter Review
Dialog shrunken on opening (3.60 KB, image/png)
2017-08-18 02:05 PDT, Richard Marti (:Paenglab)
no flags Details
react-calendar-properties-dialog.patch (102.97 KB, patch)
2017-08-18 04:44 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
react-calendar-properties-dialog.patch (101.28 KB, patch)
2017-08-19 17:40 PDT, Arshad Khan [:arshad]
Paenglab: feedback+
Details | Diff | Splinter Review
react-calendar-properties-dialog(html).patch (233.30 KB, patch)
2017-08-21 08:43 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
react-prop-dialog-html.patch (98.55 KB, patch)
2017-08-21 12:49 PDT, Arshad Khan [:arshad]
Paenglab: feedback+
Details | Diff | Splinter Review
react-cal-prop-dialog-html.patch (234.21 KB, patch)
2017-08-22 05:19 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
react-cal-prop-dialog.patch (234.27 KB, patch)
2017-08-22 16:17 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
react-calendar-properties-dialog.patch (290.30 KB, patch)
2017-08-29 23:35 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
calpropdialog.patch (106.55 KB, patch)
2017-08-31 01:55 PDT, Arshad Khan [:arshad]
Paenglab: feedback+
Details | Diff | Splinter Review
cal-prop-dialog.patch (290.79 KB, patch)
2017-10-12 08:59 PDT, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
calendar-property-dialog.patch (106.55 KB, patch)
2018-01-08 10:14 PST, Arshad Khan [:arshad]
no flags Details | Diff | Splinter Review
Fix - v2 (112.47 KB, patch)
2018-01-27 15:23 PST, Philipp Kewisch [:Fallen] [:📆]
no flags Details | Diff | Splinter Review

Description User image Arshad Khan [:arshad] 2017-08-17 16:28:57 PDT
Created attachment 8898512 [details] [diff] [review]
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 User image Arshad Khan [:arshad] 2017-08-17 23:06:02 PDT
Created attachment 8898635 [details] [diff] [review]
react-calendar-properties-dialog.patch
Comment 2 User image Richard Marti (:Paenglab) 2017-08-18 02:03:11 PDT
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 User image Richard Marti (:Paenglab) 2017-08-18 02:05:38 PDT
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.
Comment 4 User image Arshad Khan [:arshad] 2017-08-18 02:11:02 PDT
(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 User image Arshad Khan [:arshad] 2017-08-18 04:44:02 PDT
Created attachment 8898736 [details] [diff] [review]
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 User image Jorg K (GMT+1) 2017-08-18 04:47:54 PDT
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 User image Richard Marti (:Paenglab) 2017-08-18 04:55:05 PDT
Comment on attachment 8898736 [details] [diff] [review]
react-calendar-properties-dialog.patch

The dialog is still shrunken. :(
Comment 8 User image Arshad Khan [:arshad] 2017-08-18 04:57:34 PDT
(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 User image Arshad Khan [:arshad] 2017-08-19 17:40:04 PDT
Created attachment 8899206 [details] [diff] [review]
react-calendar-properties-dialog.patch

Removed the shrunken window and overriding css issue reported by Richard Marti.
Comment 10 User image Richard Marti (:Paenglab) 2017-08-20 02:55:20 PDT
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 User image Arshad Khan [:arshad] 2017-08-21 08:43:27 PDT
Created attachment 8899501 [details] [diff] [review]
react-calendar-properties-dialog(html).patch

Removed all XUL from calendar properties dialog.
Comment 12 User image Richard Marti (:Paenglab) 2017-08-21 09:46:42 PDT
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 User image Arshad Khan [:arshad] 2017-08-21 12:49:43 PDT
Created attachment 8899579 [details] [diff] [review]
react-prop-dialog-html.patch

This patch also closes Bug 1151440 [1]

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1151440
Comment 14 User image Richard Marti (:Paenglab) 2017-08-21 13:30:29 PDT
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 User image Arshad Khan [:arshad] 2017-08-22 05:19:05 PDT
Created attachment 8899810 [details] [diff] [review]
react-cal-prop-dialog-html.patch

Changes in patch:
- Set a max width for force-disabled text
Comment 16 User image Arshad Khan [:arshad] 2017-08-22 05:21:33 PDT
(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 User image Arshad Khan [:arshad] 2017-08-22 16:17:15 PDT
Created attachment 8900031 [details] [diff] [review]
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 User image Arshad Khan [:arshad] 2017-08-29 23:35:52 PDT
Created attachment 8902538 [details] [diff] [review]
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 User image Richard Marti (:Paenglab) 2017-08-30 09:09:36 PDT
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 User image Arshad Khan [:arshad] 2017-08-31 01:55:21 PDT
Created attachment 8903074 [details] [diff] [review]
calpropdialog.patch
Comment 21 User image Richard Marti (:Paenglab) 2017-08-31 02:42:43 PDT
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 User image Arshad Khan [:arshad] 2017-08-31 04:39:02 PDT
(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 User image Arshad Khan [:arshad] 2017-08-31 04:39:52 PDT
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 User image Arshad Khan [:arshad] 2017-10-12 08:59:10 PDT
Created attachment 8917883 [details] [diff] [review]
cal-prop-dialog.patch

Added licence comment to react-utils.js file.
Comment 25 User image Richard Marti (:Paenglab) 2017-10-12 09:41:36 PDT
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 User image Arshad Khan [:arshad] 2017-10-12 11:23:04 PDT
Ok, I will try to do this asap!
Comment 27 User image Arshad Khan [:arshad] 2018-01-08 10:14:28 PST
Created attachment 8940791 [details] [diff] [review]
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 User image Jorg K (GMT+1) 2018-01-27 12:15:57 PST
Philipp, are we going to proceed with this bug (and bug 1393694), or was this a "proof of concept" only?
Comment 29 User image Philipp Kewisch [:Fallen] [:📆] 2018-01-27 13:33:45 PST
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 User image Philipp Kewisch [:Fallen] [:📆] 2018-01-27 15:23:39 PST
Created attachment 8946096 [details] [diff] [review]
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 User image Jorg K (GMT+1) 2018-01-27 15:31:31 PST
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 User image Jorg K (GMT+1) 2018-01-27 15:32:51 PST
Sorry, you moved the XUL file to .html. That doesn't convince me.
Comment 33 User image Philipp Kewisch [:Fallen] [:📆] 2018-01-27 15:35:24 PST
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 User image Jorg K (GMT+1) 2018-01-27 15:41:38 PST
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.

Note You need to log in before you can comment on or make changes to this bug.