Closed
Bug 1294534
Opened 8 years ago
Closed 8 years ago
[GSOC 2016] Implement the new design of the UI for editing events and tasks in HTML
Categories
(Calendar :: Dialogs, enhancement)
Calendar
Dialogs
Tracking
(Not tracked)
RESOLVED
FIXED
5.3
People
(Reporter: pmorris, Assigned: pmorris, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
|
(deleted),
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
Now that we support editing events and tasks in a tab or a window dialog (bug 1277972), we can implement the new design for this UI using HTML and React.js instead of XUL. See https://wiki.mozilla.org/Calendar:Event_in_a_Tab
|
Assignee | |
Comment 1•8 years ago
|
||
WIP patch showing where things currently stand with HTML implementation work. The basics are there, especially getting up and running with HTML/React.js and a responsive design but otherwise things are still very rough and not usable yet. This patch depends on the patch from bug 1277972.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #8780282 -
Flags: feedback?(philipp)
Attachment #8780282 -
Flags: feedback?(makemyday)
|
|
Assignee | |
Updated•8 years ago
|
Blocks: event-in-a-tab
|
||
Comment 3•8 years ago
|
||
Comment on attachment 8780282 [details] [diff] [review]
event-task-html-ui-aug11.patch
Review of attachment 8780282 [details] [diff] [review]:
-----------------------------------------------------------------
Just a brief first codewise feedback for now - I still need to test drive this later. In general the patch is looking good, the changes seem to be less extensive than I expected them before.
There's some of code you've commented out in the patch - is that the original code of the previous implementation or some experimental code you've decided to not use for now?
::: calendar/base/content/calendar-item-editing.js
@@ +442,5 @@
> args.onOk = callback;
> args.job = job;
> args.initialStartDateValue = (initialDate || getDefaultStartDate());
> args.inTab = Preferences.get("calendar.item.editInTab", false);
> + args.htmlTabOrWindow = Preferences.get("calendar.item.editInHtmlTabOrWindow", false);
Make the pref name a little bit more comprehensive and maybe less implementation centric like calendar.item.useNewItemUI or something like that.
::: calendar/lightning/content/html-item-editing/html-item-editing.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This file mostly contains functions copied (from lightning-item-iframe.js
> +// etc.) and modified to make them work with react.js implementation.
As the changes for react seem to be less invasive than I expected them in the first place, it's probably worth to reconsider to include the react specific changes directly in lightning-item-iframe.js using a switch. That way we can avoid the burden to maintain both files when we need to adapt something in lightning-item-iframe.js.
@@ +295,5 @@
> + }
> + */
> + let calendarToUse = aItem.calendar || window.arguments[0].calendar;
> + let calendars = getCalendarsForMenu(
> + aItem,
Whitespace - there are also several other occurences in this file and others (they are highlighted in the splinter view here on bmo, so you can easily identify them if you editor doesn't support that).
@@ +497,5 @@
> + document.getElementById('container')
> + );
> +}
> +
> +// Copied and modified from calendar-ui-utils.js > appendCalendarItems
You load calendar-ui-utils.js in lightnin-item-iframe.html anyway, so here the same as in the above comment for the duplicating code from lightning-item-iframe.js would apply.
::: calendar/lightning/content/html-item-editing/lightning-item-iframe.html
@@ +3,5 @@
> + <head>
> + <meta charset='utf-8'>
> + <title>Event in a Tab</title>
> + <style>
> + /* styles to be moved to a css file, eventually */
Yes, please do so. You can start with a version common to all OS, but maybe OS specific implementations are needed. And the css file should then be stored with the other theme files.
::: calendar/lightning/content/html-item-editing/react-code.js
@@ +7,5 @@
> +var gTopComponent = null;
> +
> +// to save typing and increase readability,
> +var R = React;
> +var RD = React.DOM;
Leave it with the original names here. We will introduce an Eslint rule to avoid too short names except of some exceptions.
@@ +82,5 @@
> +
> +var Dropdown = R.createClass({
> + handleChange: function(event) {
> + // GSOC
> + // console.log('event.target.value', event.target.value);
If you want to log something, make sure you've loaded modules/calUtils.jsm before, otherwise this will fail. If you just want to use that will developing, dump() should also work to write to the console, iirc.
::: calendar/lightning/content/html-item-editing/react.min.js
@@ +7,5 @@
> + * This source code is licensed under the BSD-style license found in the
> + * LICENSE file in the root directory of this source tree. An additional grant
> + * of patent rights can be found in the PATENTS file in the same directory.
> + *
> + */
Do we need to maintain a separate copy of the react code or is this already included somewhere in mozilla-central and we can use just that - devtools seems to have a copy at least (maybe it would make sense to move that to a more general place then)? The same would apply for react-dom.min.js.
In case we need to store our own copy, we also need to make sure we have the according license text stored at an appropriate place in the tree.
::: calendar/lightning/content/lightning.js
@@ +154,5 @@
> pref("calendar.item.editInTab", false);
>
> +// Edit events and tasks in an HTML tab or window
> +pref("calendar.item.editInHtmlTabOrWindow", false);
> +
See the comment above for renaming the pref.
Attachment #8780282 -
Flags: feedback?(makemyday) → feedback+
|
|
||
Comment 4•8 years ago
|
||
I cannot close a new event tab again with this patch applied and the pref enabled. I even cannot shutdown TB once a new event tab was opened, I had to kill the process.
|
|
Assignee | |
Comment 5•8 years ago
|
||
(In reply to [:MakeMyDay] from comment #3)
> Comment on attachment 8780282 [details] [diff] [review]
> event-task-html-ui-aug11.patch
>
> Review of attachment 8780282 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Just a brief first codewise feedback for now - I still need to test drive
> this later. In general the patch is looking good, the changes seem to be
> less extensive than I expected them before.
Thanks, I expect that the changes will get a lot more extensive before we're done...
> There's some of code you've commented out in the patch - is that the
> original code of the previous implementation or some experimental code
> you've decided to not use for now?
Almost all of this is previous implementation code, just as a reminder of what the previous code was doing until I get the new code to replace it working fully. There may be a few instances of new code that's still commented out.
> ::: calendar/base/content/calendar-item-editing.js
> @@ +442,5 @@
> > args.onOk = callback;
> > args.job = job;
> > args.initialStartDateValue = (initialDate || getDefaultStartDate());
> > args.inTab = Preferences.get("calendar.item.editInTab", false);
> > + args.htmlTabOrWindow = Preferences.get("calendar.item.editInHtmlTabOrWindow", false);
>
> Make the pref name a little bit more comprehensive and maybe less
> implementation centric like calendar.item.useNewItemUI or something like
> that.
Ok, that sounds good.
> ::: calendar/lightning/content/html-item-editing/html-item-editing.js
> @@ +2,5 @@
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// This file mostly contains functions copied (from lightning-item-iframe.js
> > +// etc.) and modified to make them work with react.js implementation.
>
> As the changes for react seem to be less invasive than I expected them in
> the first place, it's probably worth to reconsider to include the react
> specific changes directly in lightning-item-iframe.js using a switch. That
> way we can avoid the burden to maintain both files when we need to adapt
> something in lightning-item-iframe.js.
I assume you mean merging "html-item-editing.js" and "lightning-item-iframe.js" and keeping "react-code.js" separate? I think there will be more invasiveness as more work is done. So I'm not sure whether combining files at this point is better or not. Hard to say without knowing just how invasive things will get.
>
> @@ +295,5 @@
> > + }
> > + */
> > + let calendarToUse = aItem.calendar || window.arguments[0].calendar;
> > + let calendars = getCalendarsForMenu(
> > + aItem,
>
> Whitespace - there are also several other occurences in this file and others
> (they are highlighted in the splinter view here on bmo, so you can easily
> identify them if you editor doesn't support that).
Yes, I'll fix these. (My editor shows them but it is pretty subtle, so I use hg diff to make them stand out. Just hadn't done it yet for this patch in the interest of time.)
> @@ +497,5 @@
> > + document.getElementById('container')
> > + );
> > +}
> > +
> > +// Copied and modified from calendar-ui-utils.js > appendCalendarItems
>
> You load calendar-ui-utils.js in lightnin-item-iframe.html anyway, so here
> the same as in the above comment for the duplicating code from
> lightning-item-iframe.js would apply.
So putting these modifications in calendar-ui-utils.js, with a conditional switch.
> ::: calendar/lightning/content/html-item-editing/lightning-item-iframe.html
> @@ +3,5 @@
> > + <head>
> > + <meta charset='utf-8'>
> > + <title>Event in a Tab</title>
> > + <style>
> > + /* styles to be moved to a css file, eventually */
>
> Yes, please do so. You can start with a version common to all OS, but maybe
> OS specific implementations are needed. And the css file should then be
> stored with the other theme files.
Ok.
> ::: calendar/lightning/content/html-item-editing/react-code.js
> @@ +7,5 @@
> > +var gTopComponent = null;
> > +
> > +// to save typing and increase readability,
> > +var R = React;
> > +var RD = React.DOM;
>
> Leave it with the original names here. We will introduce an Eslint rule to
> avoid too short names except of some exceptions.
Ok.
> @@ +82,5 @@
> > +
> > +var Dropdown = R.createClass({
> > + handleChange: function(event) {
> > + // GSOC
> > + // console.log('event.target.value', event.target.value);
>
> If you want to log something, make sure you've loaded modules/calUtils.jsm
> before, otherwise this will fail. If you just want to use that will
> developing, dump() should also work to write to the console, iirc.
Ok.
> ::: calendar/lightning/content/html-item-editing/react.min.js
> @@ +7,5 @@
> > + * This source code is licensed under the BSD-style license found in the
> > + * LICENSE file in the root directory of this source tree. An additional grant
> > + * of patent rights can be found in the PATENTS file in the same directory.
> > + *
> > + */
>
> Do we need to maintain a separate copy of the react code or is this already
> included somewhere in mozilla-central and we can use just that - devtools
> seems to have a copy at least (maybe it would make sense to move that to a
> more general place then)? The same would apply for react-dom.min.js.
Sounds fine to me. I'll see if I can find the devtools copy.
> In case we need to store our own copy, we also need to make sure we have the
> according license text stored at an appropriate place in the tree.
Ok.
> I cannot close a new event tab again with this patch applied and the pref
> enabled. I even cannot shutdown TB once a new event tab was opened, I had
> to kill the process.
Yes, sorry about that. I should have mentioned this as part of my "still very rough and not usable yet" disclaimer when uploading the patch. In the next patch I'll work on fixing this.
|
|
Assignee | |
Comment 6•8 years ago
|
||
In this patch:
- html/react code included in lightning-item-iframe.js with conditional switch
- new pref name
- separate css file
- removed aliases (R. -> React.)
- uses devtools copies of react.js files
- tabs can close, TB can quit
- formatting / documentation improvements
- new UI works in a window
I ran the mozmill tests, but it looks like some of these are failing for previous changesets right now. This patch does not cause additional failures at least. Still needs a try server run. I might be able to get that started tonight, or tomorrow if not.
Attachment #8782260 -
Flags: review?(makemyday)
|
|
Assignee | |
Comment 7•8 years ago
|
||
This version plays nicely with other pending patches, see comment 3 here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1295392#c3
Attachment #8780282 -
Attachment is obsolete: true
Attachment #8782260 -
Attachment is obsolete: true
Attachment #8780282 -
Flags: feedback?(philipp)
Attachment #8782260 -
Flags: review?(makemyday)
Attachment #8783208 -
Flags: review?(makemyday)
|
|
||
Comment 8•8 years ago
|
||
Try push with current patch on top of the events-tasks-menu-items patch from 1283623:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=447d3a5cd8bd054d9752e04ab635b4c62377122d
|
|
Assignee | |
Comment 9•8 years ago
|
||
This version passes all mozmill tests by fixing some tests to fit the new iframe loading situation. (Iframe content is loaded after its parent content for window dialogs, and tabs too in a subsequent patch.) Also fixes a minor error message appearing in the console log.
Attachment #8783208 -
Attachment is obsolete: true
Attachment #8783208 -
Flags: review?(makemyday)
Attachment #8783652 -
Flags: review?(philipp)
|
|
Assignee | |
Comment 10•8 years ago
|
||
And once more so it cleanly applies on top of the latest version of patch from bug 1283623.
Attachment #8783652 -
Attachment is obsolete: true
Attachment #8783652 -
Flags: review?(philipp)
Attachment #8783660 -
Flags: review?(philipp)
|
|
Assignee | |
Comment 11•8 years ago
|
||
And again, this time with the react-code.js and lightning-item-iframe.html files included. They were accidentally omitted from the previous version. Apologies for the extra emails.
Attachment #8783660 -
Attachment is obsolete: true
Attachment #8783660 -
Flags: review?(philipp)
Attachment #8783665 -
Flags: review?(philipp)
|
||
Comment 12•8 years ago
|
||
Comment on attachment 8783665 [details] [diff] [review]
event-task-html-ui-aug22-third.patch
Review of attachment 8783665 [details] [diff] [review]:
-----------------------------------------------------------------
I think this is good to go, thanks for taking care! r=philipp
Attachment #8783665 -
Flags: review?(philipp) → review+
|
|
||
Comment 13•8 years ago
|
||
Severity: normal → enhancement
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.3
You need to log in
before you can comment on or make changes to this bug.
Description
•