Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPII-2358: configurable transitions for line charts, removal of postinstall script #19

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@waharnum
Copy link
Member

waharnum commented May 29, 2017

This PR contains two changes originating in the Nexus science lab visualization work:

  1. Update transitions for the line chart elements (lines, areas and points) are now configurable through an invoker-based system. This allows end users to implement their own styles of transition which may be more suitable to different kinds of applications. This is described in more detail at https://issues.gpii.net/browse/GPII-2358

  2. The postinstall script has been removed from package.json; as we use the chartAuthoring library as a dependency in more places, having the postinstall run continues to be an irritation that must be overcome by specialized instructions. Eventually, as described at https://issues.fluidproject.org/browse/FLOE-509, we will divide the chartAuthoring functionality into a number of separate libraries, and have a separate repo of demos that use the new "materialization toolkit" described in FLOE-509 to author multimodal charts.

@waharnum

This comment has been minimized.

Copy link
Member Author

waharnum commented Jun 16, 2017

@cindyli, when you have some free time can you review these changes?


Requirements:
* `npm`

Steps:
* `npm install`
* postinstall should run the `grunt installFrontEnd` task after installation completes
* Run the `grunt installFrontEnd` task after installation completes

This comment has been minimized.

Copy link
@cindyli

cindyli Jun 21, 2017

Member

Probably add a little "note" paragraph to explain why running grunt installFrontEnd is no longer a npm postinstall step so it won't be questioned next time. Thanks.


// Uses D3's default transition; this results in the
// "wriggle" described at https://bost.ocks.org/mike/path/
floe.chartAuthoring.lineChart.timeSeries.area.updateArea.defaultTransition = function (chartLineAreaPaths, area, width, transitionLength) {

This comment has been minimized.

Copy link
@cindyli

cindyli Jun 21, 2017

Member

Do you think if it makes sense to state in the code comment that this is for the "real time" use case, such as working with the dynamic nexus data input.

@@ -63,6 +63,9 @@ https://raw.githubusercontent.com/fluid-project/chartAuthoring/master/LICENSE.tx
removeArea: {
funcName: "floe.chartAuthoring.lineChart.timeSeries.area.removeArea",
args: ["{that}"]
},
transitionArea: {
funcName: "floe.chartAuthoring.lineChart.timeSeries.area.updateArea.paginateTransition"

This comment has been minimized.

Copy link
@cindyli

cindyli Jun 21, 2017

Member

The invoker is hooked up with paginateTransition function by default while the other transition function is named defaultTransition. Can we have a better name for the latter, and add a code comment to describe these two alternative transition functions and what they are for.

The same comment applies to the usage of paginateTransition and defaultTransition in other js files: lineChartTimeSeriesLine.js and lineChartTimeSeriesPoints.js.

@cindyli

This comment has been minimized.

Copy link
Member

cindyli commented Jun 21, 2017

Would be helpful to add tests to demonstrate the effect of paginateTransition and defaultTransition. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.