Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124113002:
Fix chart legends. (Closed)
|
Created:
4 years, 5 months ago by benjhayden Modified:
4 years, 5 months ago CC:
catapult-reviews_chromium.org, tracing-review_chromium.org Base URL:
https://github.com/catapult-project/catapult.git@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
DescriptionFix chart legends.
Currently, chart legend keys are colored text that can be obscured by data bars.
This CL moves legend keys to the right margin of the chart,
elides them with text-overflow,
allows them to be analysis-links,
adds color to analysis-links,
adds checkboxes next to legend-keys for optional data series,
hides data series when those checkboxes are unchecked,
refactors seriesKeys_ into a Map of DataSeries objects "seriesByKey_",
allows hiding either axis in 2d charts,
cleans out a dead method in pie_chart,
and fixes hovering over value text in pie_chart.
Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/9b7295966d4c3edaad4f7e21bc51f61b44e32876
Patch Set 1 #
Total comments: 29
Patch Set 2 : comments #
Total comments: 20
Patch Set 3 : finished refactoring DataSeries Map #
Messages
Total messages: 25 (16 generated)
Description was changed from ========== Fix chart legends Currently, chart legends are colored text. However, for richer use cases, they need to be links with checkboxes. ========== to ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, allows them to be analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, cleans out a dead method in pie_chart, fixes hovering over value text in pie_chart, allows hiding either axis in 2d charts. ==========
Description was changed from ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, allows them to be analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, cleans out a dead method in pie_chart, fixes hovering over value text in pie_chart, allows hiding either axis in 2d charts. ========== to ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ==========
Description was changed from ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ========== to ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix chart legends Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ========== to ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ==========
Description was changed from ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. ========== to ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ==========
benjhayden@chromium.org changed reviewers: + charliea@chromium.org, eakuefner@chromium.org, nduca@chromium.org
PTAL :-)
Nice cleanup! lgtm if Charlie is happy.
Didn't have time to review the full CL quite yet, but thought it might save some time if I sent out my initial comments. Thank you for taking this on! This definitely looks like some code that needs an overhaul. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/bar_chart.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:27: this.xCushion_ = 1; Would you mind adding a tiny comment about what a cushion is? Something like: // The number of horizontal pixels between data points. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:29: <input type=checkbox id="checkbox" checked> Am I right that the goal of this is to toggle #link and #span visibility when #checkbox is checked? If so, I think it's preferred to do this with CSS because it separates out the display logic a little better: In the style, you can do: <style> #checkbox {...} // Hide both #span and #link by default. #span, #link { display: none; } // Show #link when #checkbox is checked. #checkbox[checked] ~ #link { display: inline-block; } // Show #span when #checkbox is unchecked. #checkbox:not([checked]) ~ #span { display: inline-block; } </style> Note that the selector "#checkbox[checked] ~ #link" means select #link, but only when it's the sibling of a #checkbox element with the attribute "checked". You can also definitely do this without the first "display: none;" block and instead write selectors about when to hide the elements rather than when to show them. However, I think this version is a little more logical because it's not in terms of double negatives (if that makes sense). There are all sorts of cool things you can do with CSS attribute (https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors) and sibling (https://developer.mozilla.org/en-US/docs/Web/CSS/General_sibling_selectors) selectors :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:31: <div id="span"></div> Is there a more descriptive name that we could use for this? It's really confusing, given that "span" is another HTML element that's the same as a div but with a default of display: inline instead of display: block. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:39: for (var node of nodes) { I'm not sure I understand what this is doing: could you clarify? https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:55: this.$.span.textContent = t; Maybe: this.$.span.textContent = this.$.link.textContent = t; https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:57: this.updateContents_(); I think that, if you do toggle visibility in full CSS, you can get rid of updateContents_ altogether https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:64: set optional(o) { Could you add a comment about what an optional legend key is? https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:73: this.$.span.style.color = c; Same about maybe: this.$.span.style.color = this.$.link.color = c; https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:157: this.legendTargets_ = {}; Could you add a comment clarifying what legend targets are? https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:244: customizeLegendTargets: function(delta) { nit: could you describe what each of the customize__ methods do? It's not completely clear to me what exactly we're customizing https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:245: tr.b.iterItems(delta, function(key, value) { note: I'm hoping to update the d8 binary by tomorrow that will allow us to do: for (var [key, value] of delta) this.legendTargets_[key] = value; Once this is possible, it should be the preferred way of iterating through a map (based on the discussions from that enormous email chain) https://codereview.chromium.org/2133453002/ is the code review where I'm submitting it.
https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/bar_chart.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:27: this.xCushion_ = 1; On 2016/07/07 at 21:35:59, charliea wrote: > Would you mind adding a tiny comment about what a cushion is? Something like: > > // The number of horizontal pixels between data points. No explanation for this field can be tiny, so I added a large comment. :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:29: <input type=checkbox id="checkbox" checked> On 2016/07/07 at 21:35:59, charliea wrote: > Am I right that the goal of this is to toggle #link and #span visibility when #checkbox is checked? Nope. :-) The checkbox toggles whether the data series for this legend key is displayed. I added some comments. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:31: <div id="span"></div> On 2016/07/07 at 21:36:00, charliea wrote: > Is there a more descriptive name that we could use for this? It's really confusing, given that "span" is another HTML element that's the same as a div but with a default of display: inline instead of display: block. I changed it back to a span tag, since its display is set in the style block anyway. (I had changed it from a span to a div in order to make text-overflow work, but I had to change the display to inline-block when I added the checkbox and just forgot to change the tag name back to agree with the id.) I'm not sure what else to call it. The way that this legend-key polymer-element works is that it always displays a single piece of text, and that text is either clickable or not. When it's clickable, it's an analysis-link; when it's not clickable, it's just... a span of text? So the id "span" is meant to imply "as opposed to a link". Open to suggestions. :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:39: for (var node of nodes) { On 2016/07/07 at 21:35:59, charliea wrote: > I'm not sure I understand what this is doing: could you clarify? Oh I guess I don't really need this. I added it while debugging the layout with the checkbox and the span/link, under the hypothesis that the whitespace between tags in the template was causing line breaks. (I think it must have been the display:inline-block that fixed that.) I removed this loop, and it looks like the whitespace between tags in the template does add a few pixels of horizontal space between the elements, but folks will probably like it better this way. I don't think this CL is the right time to optimize a few pixels here or there. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:55: this.$.span.textContent = t; On 2016/07/07 at 21:36:00, charliea wrote: > Maybe: > > this.$.span.textContent = this.$.link.textContent = t; Eh, I find that makes it hard to see where link.textContent is set? :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:64: set optional(o) { On 2016/07/07 at 21:36:00, charliea wrote: > Could you add a comment about what an optional legend key is? Done. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:157: this.legendTargets_ = {}; On 2016/07/07 at 21:36:00, charliea wrote: > Could you add a comment clarifying what legend targets are? I added it by customizeLegendTargets() if that's ok? https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:245: tr.b.iterItems(delta, function(key, value) { On 2016/07/07 at 21:35:59, charliea wrote: > note: I'm hoping to update the d8 binary by tomorrow that will allow us to do: > > for (var [key, value] of delta) > this.legendTargets_[key] = value; You'll also need tr.b.entries(dict), right? Chrome says Uncaught TypeError: dict[Symbol.iterator] is not a function > > Once this is possible, it should be the preferred way of iterating through a map (based on the discussions from that enormous email chain) > > https://codereview.chromium.org/2133453002/ is the code review where I'm submitting it. \o/
Got it. Everything makes much more sense to me now! Added a few remaining comments. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/bar_chart.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:27: this.xCushion_ = 1; On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:35:59, charliea wrote: > > Would you mind adding a tiny comment about what a cushion is? Something like: > > > > // The number of horizontal pixels between data points. > > No explanation for this field can be tiny, so I added a large comment. :-) Haha, sounds good. If a longer description is required, a longer description is perfect :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:29: <input type=checkbox id="checkbox" checked> On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:35:59, charliea wrote: > > Am I right that the goal of this is to toggle #link and #span visibility when > #checkbox is checked? > > Nope. :-) > The checkbox toggles whether the data series for this legend key is displayed. > I added some comments. Doh. I understand it now. Didn't read through quite carefully enough before getting excited about CSS3. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:31: <div id="span"></div> On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:36:00, charliea wrote: > > Is there a more descriptive name that we could use for this? It's really > confusing, given that "span" is another HTML element that's the same as a div > but with a default of display: inline instead of display: block. > > I changed it back to a span tag, since its display is set in the style block > anyway. (I had changed it from a span to a div in order to make text-overflow > work, but I had to change the display to inline-block when I added the checkbox > and just forgot to change the tag name back to agree with the id.) > I'm not sure what else to call it. > The way that this legend-key polymer-element works is that it always displays a > single piece of text, and that text is either clickable or not. When it's > clickable, it's an analysis-link; when it's not clickable, it's just... a span > of text? > So the id "span" is meant to imply "as opposed to a link". > Open to suggestions. :-) Ahhh, okay. What about just a <label id="label"> element? I think it's effectively a span, but it conveys the purpose a little more. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:39: for (var node of nodes) { On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:35:59, charliea wrote: > > I'm not sure I understand what this is doing: could you clarify? > > Oh I guess I don't really need this. I added it while debugging the layout with > the checkbox and the span/link, under the hypothesis that the whitespace between > tags in the template was causing line breaks. (I think it must have been the > display:inline-block that fixed that.) > I removed this loop, and it looks like the whitespace between tags in the > template does add a few pixels of horizontal space between the elements, but > folks will probably like it better this way. I don't think this CL is the right > time to optimize a few pixels here or there. Acknowledged. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:55: this.$.span.textContent = t; On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:36:00, charliea wrote: > > Maybe: > > > > this.$.span.textContent = this.$.link.textContent = t; > > Eh, I find that makes it hard to see where link.textContent is set? :-) Acknowledged. Don't think we have any firm style for this in place. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:156: this.seriesKeys_ = undefined; It feels a little bit strange to have four different series-related fields, keys, targets, optional, and enabled, that are all really attributes about a series, but which are held in separate fields. What do you think about having a super-simple non-exported class, ChartSeries, that encapsulates these? Something like: var ChartSeries = function(key, target, enabled, optional) { this.key = key; this.target = target; this.enabled = enabled; this.optional = optional; } and then you can store all of these in an ES6 Map (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...) keyed by the series key: this.seriesByKey_ = new Map(); so that, when you need to know if a given series is enabled, you can just do: this.seriesByKey_['foo'].enabled Not sure what your thoughts on for this, but it seems to logically consolidate the data being stored. Either way, I think for most of these fields, we should use ES6 maps rather than objects, because what we're really after is the dictionary-like functionality of javascript objects. https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:157: this.legendTargets_ = {}; On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:36:00, charliea wrote: > > Could you add a comment clarifying what legend targets are? > > I added it by customizeLegendTargets() if that's ok? SGTM https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:157: this.legendTargets_ = {}; nit: I think this should probably now be an ES6 Map rather than a dictionary: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:245: tr.b.iterItems(delta, function(key, value) { On 2016/07/07 22:40:18, benjhayden_chromium wrote: > On 2016/07/07 at 21:35:59, charliea wrote: > > note: I'm hoping to update the d8 binary by tomorrow that will allow us to do: > > > > for (var [key, value] of delta) > > this.legendTargets_[key] = value; > > You'll also need tr.b.entries(dict), right? > Chrome says Uncaught TypeError: dict[Symbol.iterator] is not a function > > > > > Once this is possible, it should be the preferred way of iterating through a > map (based on the discussions from that enormous email chain) > > > > https://codereview.chromium.org/2133453002/ is the code review where I'm > submitting it. > > \o/ Ahhh, yep, you're right. Forgot this was a dict and not a map. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/bar_chart.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:28: // BarChart allows bars to have arbitrary width. Bars need not all be the Maybe the first sentence could be "BarChart allows bars to have arbitrary, non-uniform widths."? https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:32: // This is the width (in the x-axis domain) of the last bar. "width" is probably sufficient (without the "in the x-axis-domain"): otherwise, I think it'd generally be considered "height" instead https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:35: // non-zero number so that the width of the only bar will not be zero. After spending about 10 minutes looking at the code, I've come to the same conclusion as you: this is a very strange and complicated variable. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (left): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:186: // dipping below the x-axis and continue on outside of the viewport. Thanks for doing this! https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:44: * Dispatch an event when the checkbox is toggled. nit: IDT we usually given each sentence its own line like this https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:47: onCheckboxChange_: function() { maybe: onDataSeriesToggled? https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:48: var event = new tr.ui.b.DataSeriesEnableChangeEvent(); maybe: DataSeriesToggledEvent? https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:72: this.$.checkbox.style.display = optional ? 'inline-block' : 'none'; not actually anything to fix, but later on, it'd be great if we could use Polymer data binding here so that we could do: <input type=checkbox id="checkbox" hidden="{{!optional}}" checked> That'd get rid of this method completely. I don't think we can do that quite yet because we're not using Polymer attributes just about anywhere in our UI https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:85: * When target is defined, span is hidden and link is shown. same nit about line splits not being necessary https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:260: * Legend keys can be clickable links instead of plain text. Same nit about line splits https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:260: * Legend keys can be clickable links instead of plain text. My feeling is that this might be a bit heavy on the documentation for the method, which will discourage people form reading it. I think that a shortened version of this, like: " Legends keys can be links that select a group of events, or "target", when clicked. To set the legend key for the "foo" data series associated with a given event array, you can do: customizeLegendTargets({foo: events}); " would get the point across but be shorter. Totally up to you, though. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:279: * Optional data series can be enabled and disabled using checkboxes. Same, both about line splits and about length of the comment. I think something like: " Data series can be optional, which allows their visibility in the chart to be toggled via a checkbox. To set the optionality for the "foo" data series, you can do: customizeOptionalSeries({foo: true}); " would probably suffice. Up to you though. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:303: * Data series can be enabled and disabled. Same nit about line splits https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:323: if (this.seriesKeys_ && nit: generally, chromium code tries to have the "happy, normal" path be at the lowest level of indentation if it's not super inconvenient. For this case, it seems like "there are multiple series to copy" would probably be considered the normal path, and I think the more chromium-idiomatic version can be written in the same number of lines: if (!this.seriesKeys_ || this.seriesKeys_.length < 2) return []; return this.seriesKeys_.slice(); https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:376: .append('foreignObject') neat! didn't know you could do this in d3 https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base_2d.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base_2d.html:58: get hideXAxis() { I'm always really conflicted about stuff like this: on one hand, I prefer to have properties/fields based on positives ("chart.showXAxis") rather than negatives ("chart.hideXAxis"). I think it's generally easier to reason about, especially when a second negative is involved. e.g. chart.showXAxis = myCondition; // Easy to understand and chart.showXAxis = !myCondition; // Easy to understand rather than chart.hideXAxis = someCondition; // Still pretty easy to understand and chart.hideXAxis = !someCondition; // Hard to understand It seems like the "positive" version is strictly better. However, HTML5 wants to prove me wrong, because they added a "hidden" attribute that toggles element visibility rather than a "visible" attribute, and maybe it makes sense to follow their lead. Not really offering any solutions, just some thoughts. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/pie_chart.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/pie_chart.html:206: .on('mouseenter', function(d, i) { Wish we had arrow functions working: we could just do .on('mouseenter', (d, i) => { var origData = this.data_[i]; this.pushTempHighlightedLegendKey(origData.label); }) because arrow functions don't change the binding of "this" from the outer scope. But... they don't. Sad face.
Description was changed from ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to them for optional data series, hides data series when checkboxes are unchecked, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ========== to ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to legend-keys for optional data series, hides data series when those checkboxes are unchecked, refactors seriesKeys_ into a Map seriesByKey_ containing DataSeries objects, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ==========
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to legend-keys for optional data series, hides data series when those checkboxes are unchecked, refactors seriesKeys_ into a Map seriesByKey_ containing DataSeries objects, allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ========== to ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to legend-keys for optional data series, hides data series when those checkboxes are unchecked, refactors seriesKeys_ into a Map of DataSeries objects "seriesByKey_", allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ==========
Patchset #3 (id:120001) has been deleted
PTAL :-) https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/20001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:31: <div id="span"></div> On 2016/07/08 at 19:10:38, charliea wrote: > On 2016/07/07 22:40:18, benjhayden_chromium wrote: > > On 2016/07/07 at 21:36:00, charliea wrote: > > > Is there a more descriptive name that we could use for this? It's really > > confusing, given that "span" is another HTML element that's the same as a div > > but with a default of display: inline instead of display: block. > > > > I changed it back to a span tag, since its display is set in the style block > > anyway. (I had changed it from a span to a div in order to make text-overflow > > work, but I had to change the display to inline-block when I added the checkbox > > and just forgot to change the tag name back to agree with the id.) > > I'm not sure what else to call it. > > The way that this legend-key polymer-element works is that it always displays a > > single piece of text, and that text is either clickable or not. When it's > > clickable, it's an analysis-link; when it's not clickable, it's just... a span > > of text? > > So the id "span" is meant to imply "as opposed to a link". > > Open to suggestions. :-) > > Ahhh, okay. What about just a <label id="label"> element? I think it's effectively a span, but it conveys the purpose a little more. Actually, <label> elements are intended to be labels *for* specific input elements, so that clicking on them is the same as clicking on the input element. Here, the obvious input element is the checkbox, so that clicking the label will toggle the checkbox. However, we need to be careful to only set htmlFor when the data series is optional so that clicking the label won't toggle the data series if the data series isn't optional. I was worried that making the label clickable might be confusing if the same chart has both data series with legend targets and without, but I think that analysis-link's underline is a clear enough indication of whether clicking the key will toggle the checkbox or dispatch an event to be handled by the embedder. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/bar_chart.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/bar_chart.html:32: // This is the width (in the x-axis domain) of the last bar. On 2016/07/08 at 19:10:38, charliea wrote: > "width" is probably sufficient (without the "in the x-axis-domain"): otherwise, I think it'd generally be considered "height" instead I see you have not had the joy of grokking d3's Scale concept. There are two separate horizontal coordinate systems: data (xScale.domain) and pixel (xScale.range). I clarified the comment to say that xCushion's coordinate system is the xScale's domain, not the xScale's range. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base.html:323: if (this.seriesKeys_ && On 2016/07/08 at 19:10:39, charliea wrote: > nit: generally, chromium code tries to have the "happy, normal" path be at the lowest level of indentation if it's not super inconvenient. For this case, it seems like "there are multiple series to copy" would probably be considered the normal path, and I think the more chromium-idiomatic version can be written in the same number of lines: > > if (!this.seriesKeys_ || > this.seriesKeys_.length < 2) > return []; > > return this.seriesKeys_.slice(); This method evaporated in the seriesByKey_ refactoring. https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... File tracing/tracing/ui/base/chart_base_2d.html (right): https://codereview.chromium.org/2124113002/diff/40001/tracing/tracing/ui/base... tracing/tracing/ui/base/chart_base_2d.html:58: get hideXAxis() { On 2016/07/08 at 19:10:39, charliea wrote: > I'm always really conflicted about stuff like this: on one hand, I prefer to have properties/fields based on positives ("chart.showXAxis") rather than negatives ("chart.hideXAxis"). I think it's generally easier to reason about, especially when a second negative is involved. > > e.g. > chart.showXAxis = myCondition; // Easy to understand > and > chart.showXAxis = !myCondition; // Easy to understand > > rather than > > chart.hideXAxis = someCondition; // Still pretty easy to understand > and > chart.hideXAxis = !someCondition; // Hard to understand > > It seems like the "positive" version is strictly better. However, HTML5 wants to prove me wrong, because they added a "hidden" attribute that toggles element visibility rather than a "visible" attribute, and maybe it makes sense to follow their lead. > > Not really offering any solutions, just some thoughts. I feel the same way. I would follow google chart's lead, but their solution to this problem is to allow callers to either set the axis to be the same color as the background, or set the number of ticks to 0, or set the axis label font size to 0, none of which seem to be quite what I want here. I'm just trying to get this to look good enough for results2.html. These charts are going to continue to require work for a while, so one of those future changes will probably find an appropriate solution here?
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eakuefner@chromium.org Link to the patchset: https://codereview.chromium.org/2124113002/#ps140001 (title: "finished refactoring DataSeries Map")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to legend-keys for optional data series, hides data series when those checkboxes are unchecked, refactors seriesKeys_ into a Map of DataSeries objects "seriesByKey_", allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png ========== to ========== Fix chart legends. Currently, chart legend keys are colored text that can be obscured by data bars. This CL moves legend keys to the right margin of the chart, elides them with text-overflow, allows them to be analysis-links, adds color to analysis-links, adds checkboxes next to legend-keys for optional data series, hides data series when those checkboxes are unchecked, refactors seriesKeys_ into a Map of DataSeries objects "seriesByKey_", allows hiding either axis in 2d charts, cleans out a dead method in pie_chart, and fixes hovering over value text in pie_chart. Screenshot: https://screenshot.googleplex.com/AH3wxRygY3K.png Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:140001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Message was sent while issue was closed.
On 2016/07/12 04:39:22, commit-bot: I haz the power wrote: > Committed patchset #3 (id:140001) as > https://chromium.googlesource.com/external/github.com/catapult-project/catapu... I look at this a bit late but the checkbox feature look awesome! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
