Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1771023002:
Fix missing message-bar Element on /report and /alerts page. (Closed)
DescriptionFix missing message-bar Element on /report and /alerts page.
'overlay-message' was originally designed to be top level element and be reusable, so it should be declared outside of Polymer element.
This CL also add html, body and head tags to static/alerts.html.
BUG=catapult:#2104
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/47d899cab4fee64faa324f529c654cfd8d0e033b
Patch Set 1 #
Total comments: 4
Patch Set 2 : address comments #
Messages
Total messages: 21 (5 generated)
chrisphan@chromium.org changed reviewers: + qyearsley@chromium.org
sullivan@chromium.org changed reviewers: + sullivan@chromium.org
If we have this change as-is, it will fix the code in base-form.html, but it will break the usage in report-page.html It's pretty confusing to assume an element exists at global scope. Part of the reason to use polymer is to modularize the HTML. I think events might be a better way to factor this--anything that has a message fires an event, an the message-bar listens for them. Quinten, WDYT? https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... File dashboard/dashboard/static/report.html (left): https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... dashboard/dashboard/static/report.html:24: <title>Chrome Performance Dashboard</title> Why remove the title?
On 2016/03/07 at 23:01:29, sullivan wrote: > If we have this change as-is, it will fix the code in base-form.html, but it will break the usage in report-page.html > > It's pretty confusing to assume an element exists at global scope. Part of the reason to use polymer is to modularize the HTML. I think events might be a better way to factor this--anything that has a message fires an event, an the message-bar listens for them. > > Quinten, WDYT? That sounds reasonable to me -- I agree that an advantage of using HTML custom elements & Polymer is to modularize functionality. Although, I think that neither assuming an element exists at top-level nor using events are very simple, and confusingness is subjective. What would the change look like to use events for showing the message bar? showMessage in base-form (https://github.com/catapult-project/catapult/blob/master/dashboard/dashboard/...) would fire an event (which would bubble up through the DOM?), and message-bar would listen for that message on its parent element? https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... File dashboard/dashboard/static/alerts.html (right): https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... dashboard/dashboard/static/alerts.html:8: <head> Adding a html, head and body looks reasonable but is not currently mentioned in the CL description. Would this change be necessary to make the message bar work?
On 2016/03/07 23:01:29, sullivan wrote: > If we have this change as-is, it will fix the code in base-form.html, but it > will break the usage in report-page.html Ah forgot about report-page.html. Fixed that. > > It's pretty confusing to assume an element exists at global scope. Part of the > reason to use polymer is to modularize the HTML. I think events might be a > better way to factor this--anything that has a message fires an event, an the > message-bar listens for them. Yea, originally I was going for a singleton pattern. If I do this again, I'd give 'overlay-message' element an app global state, and then we can do this.$['message-bar'].updateContent() where ever it is initialized. Probably better if this refactor made after 1.0 upgrade. > > Quinten, WDYT? > > https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... > File dashboard/dashboard/static/report.html (left): > > https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... > dashboard/dashboard/static/report.html:24: <title>Chrome Performance > Dashboard</title> > Why remove the title?
https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... File dashboard/dashboard/static/alerts.html (right): https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... dashboard/dashboard/static/alerts.html:8: <head> On 2016/03/07 23:47:29, qyearsley wrote: > Adding a html, head and body looks reasonable but is not currently mentioned in > the CL description. Would this change be necessary to make the message bar work? It works without them. Browsers are pretty smart these days. This just made it a proper HTML doc. You want a separate CL for this? https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... File dashboard/dashboard/static/report.html (left): https://chromiumcodereview-hr.appspot.com/1771023002/diff/1/dashboard/dashboa... dashboard/dashboard/static/report.html:24: <title>Chrome Performance Dashboard</title> On 2016/03/07 23:01:29, sullivan wrote: > Why remove the title? Woops, added it's back.
On 2016/03/08 01:54:57, chrisphan wrote: > On 2016/03/07 23:01:29, sullivan wrote: > > If we have this change as-is, it will fix the code in base-form.html, but it > > will break the usage in report-page.html > > Ah forgot about report-page.html. Fixed that. > > > > > It's pretty confusing to assume an element exists at global scope. Part of the > > reason to use polymer is to modularize the HTML. I think events might be a > > better way to factor this--anything that has a message fires an event, an the > > message-bar listens for them. > > Yea, originally I was going for a singleton pattern. If I do this again, I'd > give 'overlay-message' element an app global state, and then we can do > this.$['message-bar'].updateContent() where ever it is initialized. > > Probably better if this refactor made after 1.0 upgrade. The big problem here is that we want to start working towards having reusable components that can be used anywhere in catapult, and depending on a global existing cuts down on reusability. We'll have a larger discussion about coding element style after the polymer 1.0 upgrade. I suppose it can wait until then, but I'm still pretty uncomfortable with randomly accessing elements assumed to be on the global document.
On 2016/03/08 03:56:07, sullivan wrote: > On 2016/03/08 01:54:57, chrisphan wrote: > > On 2016/03/07 23:01:29, sullivan wrote: > > > If we have this change as-is, it will fix the code in base-form.html, but it > > > will break the usage in report-page.html > > > > Ah forgot about report-page.html. Fixed that. > > > > > > > > It's pretty confusing to assume an element exists at global scope. Part of > the > > > reason to use polymer is to modularize the HTML. I think events might be a > > > better way to factor this--anything that has a message fires an event, an > the > > > message-bar listens for them. > > > > Yea, originally I was going for a singleton pattern. If I do this again, I'd > > give 'overlay-message' element an app global state, and then we can do > > this.$['message-bar'].updateContent() where ever it is initialized. > > > > Probably better if this refactor made after 1.0 upgrade. > > The big problem here is that we want to start working towards having reusable > components that can be used anywhere in catapult, and depending on a global > existing cuts down on reusability. We'll have a larger discussion about coding > element style after the polymer 1.0 upgrade. I suppose it can wait until then, > but I'm still pretty uncomfortable with randomly accessing elements assumed to > be on the global document. Here's an example fix using global Polymer state I was talking about: https://codereview.chromium.org/1767183003/
On 2016/03/08 17:55:18, chrisphan wrote: > On 2016/03/08 03:56:07, sullivan wrote: > > On 2016/03/08 01:54:57, chrisphan wrote: > > > On 2016/03/07 23:01:29, sullivan wrote: > > > > If we have this change as-is, it will fix the code in base-form.html, but > it > > > > will break the usage in report-page.html > > > > > > Ah forgot about report-page.html. Fixed that. > > > > > > > > > > > It's pretty confusing to assume an element exists at global scope. Part of > > the > > > > reason to use polymer is to modularize the HTML. I think events might be a > > > > better way to factor this--anything that has a message fires an event, an > > the > > > > message-bar listens for them. > > > > > > Yea, originally I was going for a singleton pattern. If I do this again, > I'd > > > give 'overlay-message' element an app global state, and then we can do > > > this.$['message-bar'].updateContent() where ever it is initialized. > > > > > > Probably better if this refactor made after 1.0 upgrade. > > > > The big problem here is that we want to start working towards having reusable > > components that can be used anywhere in catapult, and depending on a global > > existing cuts down on reusability. We'll have a larger discussion about coding > > element style after the polymer 1.0 upgrade. I suppose it can wait until then, > > but I'm still pretty uncomfortable with randomly accessing elements assumed to > > be on the global document. > > Here's an example fix using global Polymer state I was talking about: > > https://codereview.chromium.org/1767183003/ That CL guarantees there would only be one global message bar, but then don't you still have to find it via document.getElementById?
On 2016/03/08 18:03:32, sullivan wrote: > On 2016/03/08 17:55:18, chrisphan wrote: > > On 2016/03/08 03:56:07, sullivan wrote: > > > On 2016/03/08 01:54:57, chrisphan wrote: > > > > On 2016/03/07 23:01:29, sullivan wrote: > > > > > If we have this change as-is, it will fix the code in base-form.html, > but > > it > > > > > will break the usage in report-page.html > > > > > > > > Ah forgot about report-page.html. Fixed that. > > > > > > > > > > > > > > It's pretty confusing to assume an element exists at global scope. Part > of > > > the > > > > > reason to use polymer is to modularize the HTML. I think events might be > a > > > > > better way to factor this--anything that has a message fires an event, > an > > > the > > > > > message-bar listens for them. > > > > > > > > Yea, originally I was going for a singleton pattern. If I do this again, > > I'd > > > > give 'overlay-message' element an app global state, and then we can do > > > > this.$['message-bar'].updateContent() where ever it is initialized. > > > > > > > > Probably better if this refactor made after 1.0 upgrade. > > > > > > The big problem here is that we want to start working towards having > reusable > > > components that can be used anywhere in catapult, and depending on a global > > > existing cuts down on reusability. We'll have a larger discussion about > coding > > > element style after the polymer 1.0 upgrade. I suppose it can wait until > then, > > > but I'm still pretty uncomfortable with randomly accessing elements assumed > to > > > be on the global document. > > > > Here's an example fix using global Polymer state I was talking about: > > > > https://codereview.chromium.org/1767183003/ > > That CL guarantees there would only be one global message bar, but then don't > you still have to find it via document.getElementById? No, once a Polymer element instantiate "overlay-message-bar", it can access with $ (See report-page.html). And all instance of "overlay-message-bar" will share a global state.
On 2016/03/08 18:07:36, chrisphan wrote: > On 2016/03/08 18:03:32, sullivan wrote: > > On 2016/03/08 17:55:18, chrisphan wrote: > > > On 2016/03/08 03:56:07, sullivan wrote: > > > > On 2016/03/08 01:54:57, chrisphan wrote: > > > > > On 2016/03/07 23:01:29, sullivan wrote: > > > > > > If we have this change as-is, it will fix the code in base-form.html, > > but > > > it > > > > > > will break the usage in report-page.html > > > > > > > > > > Ah forgot about report-page.html. Fixed that. > > > > > > > > > > > > > > > > > It's pretty confusing to assume an element exists at global scope. > Part > > of > > > > the > > > > > > reason to use polymer is to modularize the HTML. I think events might > be > > a > > > > > > better way to factor this--anything that has a message fires an event, > > an > > > > the > > > > > > message-bar listens for them. > > > > > > > > > > Yea, originally I was going for a singleton pattern. If I do this > again, > > > I'd > > > > > give 'overlay-message' element an app global state, and then we can do > > > > > this.$['message-bar'].updateContent() where ever it is initialized. > > > > > > > > > > Probably better if this refactor made after 1.0 upgrade. > > > > > > > > The big problem here is that we want to start working towards having > > reusable > > > > components that can be used anywhere in catapult, and depending on a > global > > > > existing cuts down on reusability. We'll have a larger discussion about > > coding > > > > element style after the polymer 1.0 upgrade. I suppose it can wait until > > then, > > > > but I'm still pretty uncomfortable with randomly accessing elements > assumed > > to > > > > be on the global document. > > > > > > Here's an example fix using global Polymer state I was talking about: > > > > > > https://codereview.chromium.org/1767183003/ > > > > That CL guarantees there would only be one global message bar, but then don't > > you still have to find it via document.getElementById? > > No, once a Polymer element instantiate "overlay-message-bar", it can access with > $ (See report-page.html). And all instance of "overlay-message-bar" will share > a global state. Oh. That's pretty confusing; what's the reasoning for doing it that way?
On 2016/03/08 18:09:03, sullivan wrote: > On 2016/03/08 18:07:36, chrisphan wrote: > > On 2016/03/08 18:03:32, sullivan wrote: > > > On 2016/03/08 17:55:18, chrisphan wrote: > > > > On 2016/03/08 03:56:07, sullivan wrote: > > > > > On 2016/03/08 01:54:57, chrisphan wrote: > > > > > > On 2016/03/07 23:01:29, sullivan wrote: > > > > > > > If we have this change as-is, it will fix the code in > base-form.html, > > > but > > > > it > > > > > > > will break the usage in report-page.html > > > > > > > > > > > > Ah forgot about report-page.html. Fixed that. > > > > > > > > > > > > > > > > > > > > It's pretty confusing to assume an element exists at global scope. > > Part > > > of > > > > > the > > > > > > > reason to use polymer is to modularize the HTML. I think events > might > > be > > > a > > > > > > > better way to factor this--anything that has a message fires an > event, > > > an > > > > > the > > > > > > > message-bar listens for them. > > > > > > > > > > > > Yea, originally I was going for a singleton pattern. If I do this > > again, > > > > I'd > > > > > > give 'overlay-message' element an app global state, and then we can do > > > > > > this.$['message-bar'].updateContent() where ever it is initialized. > > > > > > > > > > > > Probably better if this refactor made after 1.0 upgrade. > > > > > > > > > > The big problem here is that we want to start working towards having > > > reusable > > > > > components that can be used anywhere in catapult, and depending on a > > global > > > > > existing cuts down on reusability. We'll have a larger discussion about > > > coding > > > > > element style after the polymer 1.0 upgrade. I suppose it can wait until > > > then, > > > > > but I'm still pretty uncomfortable with randomly accessing elements > > assumed > > > to > > > > > be on the global document. > > > > > > > > Here's an example fix using global Polymer state I was talking about: > > > > > > > > https://codereview.chromium.org/1767183003/ > > > > > > That CL guarantees there would only be one global message bar, but then > don't > > > you still have to find it via document.getElementById? > > > > No, once a Polymer element instantiate "overlay-message-bar", it can access > with > > $ (See report-page.html). And all instance of "overlay-message-bar" will > share > > a global state. > > Oh. That's pretty confusing; what's the reasoning for doing it that way? The intent of "overlay-message" was to manage notifications on a page, so multiple Polymer elements may use them. For example /report page notifies 'Saving state...' and bisect-form notifies triggered jobs.
lgtm We agreed offline to submit this offline and re-visit after standardizing UI practices with trace-viewer team.
On 2016/03/09 at 22:06:07, sullivan wrote: > lgtm > > We agreed offline to submit this offline and re-visit after standardizing UI practices with trace-viewer team. LGTM and agree that we could improve this later; could you also add a quick note in the description saying that it adds html, head and body elements in static/alerts.html too?
Description was changed from ========== Fix missing message-bar Element on /report and /alerts page. 'overlay-message' was originally designed to be top level element and be reusable, so it should be declared outside of Polymer element. BUG=catapult:#2104 ========== to ========== Fix missing message-bar Element on /report and /alerts page. 'overlay-message' was originally designed to be top level element and be reusable, so it should be declared outside of Polymer element. This CL also add html, body and head tags to static/alerts.html. BUG=catapult:#2104 ==========
On 2016/03/09 22:54:07, qyearsley wrote: > On 2016/03/09 at 22:06:07, sullivan wrote: > > lgtm > > > > We agreed offline to submit this offline and re-visit after standardizing UI > practices with trace-viewer team. > > LGTM and agree that we could improve this later; could you also add a quick note > in the description saying that it adds html, head and body elements in > static/alerts.html too? Done.
The CQ bit was checked by chrisphan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1771023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1771023002/20001
Message was sent while issue was closed.
Description was changed from ========== Fix missing message-bar Element on /report and /alerts page. 'overlay-message' was originally designed to be top level element and be reusable, so it should be declared outside of Polymer element. This CL also add html, body and head tags to static/alerts.html. BUG=catapult:#2104 ========== to ========== Fix missing message-bar Element on /report and /alerts page. 'overlay-message' was originally designed to be top level element and be reusable, so it should be declared outside of Polymer element. This CL also add html, body and head tags to static/alerts.html. BUG=catapult:#2104 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
