HUE-8178 [chart] Timeline refine selection

Review Request #12851 - Created April 16, 2018 and submitted

Jean-Francois Desjeans Gauthier
hue
master
HUE-8178
hue
jgauthier
commit 87aa17b99cd9e89f7fd6bd5755b18b113caf99c0
Author: jdesjean <jgauthier@cloudera.com>
Date:   Mon Apr 16 15:58:33 2018 -0700

    HUE-8178 [chart] Timeline refine selection

:100644 100644 607e89371f... cb3a536819... M	desktop/core/src/desktop/static/desktop/js/nv.d3.lineWithBrushChart.js
:100644 100644 e83bf432dc... 298c8ef2cc... M	desktop/core/src/desktop/static/desktop/js/nv.d3.multiBarWithBrushChart.js


  • 0
  • 0
  • 13
  • 2
  • 15
Description From Last Updated
  1. +1, Enrico to confirm

  2. Hidden in the UI too?

    Not ideal but fine to just force to true like this for now

    1. I have disabled the selection handler in the UI. My only concern was, if the user has saved the chart when it's loaded again I don't want the selection mode to be false?

  3. nit:

    Maybe break it on multi line:

    counts = range_pair2(
    facet['field'],
    name,
    ...
    )

  4. 
      
  1. A couple of things but lgtm!

  2. we should be able to enable/disable the selection in relationship to the fact if we have a selection handler or not. ie. in the editor, we don't have a selection handler in place so we should disable it there

    1. I have disabled the selection handler in the UI. My only concern was, if the user has saved the chart when it's loaded again I don't want the selection mode to be false?

  3. since we are at it, we should probably refactor window.chartNormalState and window.chartsUpdatingState to huePubSub so we don't pollute the window object

  4. 
      
  1. 
      
  2. Count point, actually pretty tricky, we should make sure we don't start re-updating widgets that did not change

  3. 
      
  1. 
      
  2. desktop/core/src/desktop/static/desktop/js/ko.charts.js (Diff revision 1)
     
     
     
     
     
     

    You can drop the if statements + enableSelection var as it will always call _chart.enableSelection() no matter what _options.enableSelection is set to.

  3. desktop/core/src/desktop/static/desktop/js/ko.charts.js (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
  4. 
      
  1. Looks pretty good overall! Any chance to push to a remote branch to play a bit?

  2. nit: indent

    counts = range_pair2(
    facet['field'],
    ,...
    )

  3. 
      
  1. Ship It!
  2. Only with onClick option?

    1. That's correct, if we don't handle onClick then we should not change the chart status.

  3. Dispose? Also possible issue with multiple charts?

    1. There's currently no lifecycle for the objects in the root of this file (e.g. they get created an initialization, but never get deleted). Changing that would be a bigger change, so I'll leave the original mecanism intact.
      I believe the idea behind the current implementation is to affect all the charts. Changing this would also be a bigger change.

  4. desktop/libs/dashboard/src/dashboard/templates/common_search.mako (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Nits: A bit too much js in the binding params, cleaner (and testable) to move to the view model js and will also prevent the duplication below.

  5. 
      
  1. This is nice! Just a few nits:

    Generally?:
    -1 Should keep timeline widget for right now (it auto offers only compatible fields)
    -2 There is no way to reset/clear the filter from the graph
    -3 Legend on hover can be too high (e.g. 2 group by and tens of values) and it will push down widgets below. Maybe truncate it or need a max height and scroll
    -4 It seems like we can't unselect values in the right legend list

    We could push and work on above items on quick commits on top if easier.

    Then we could see for zooming in action, x axis legend and default interfaces easy for humanes, and another way to group by

  2. desktop/libs/dashboard/src/dashboard/templates/common_search.mako (Diff revision 9)
     
     
     
     
     
     
     
     
     

    cf. top comment

  3. 
      
Review request changed

Status: Closed (submitted)

Loading...