DM-8302 options for opposite axis location: X on top, Y on right #265
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also refactored code to separate ColumnOrExpression field to use in histogram.
loitly
approved these changes
Jan 5, 2017
There was a problem hiding this comment.
Changes look good. I tried it on multiple browsers and it was working fine. I made several comments in code, but they are only questions or comment and does not need to be changed.
Unrelated to this pull request, I noticed that histogram chart does not have a line border on the y-axis label.
| let doUpdate = false; | ||
| if (height !== nextProps.height || width !== nextProps.width ) { | ||
| chart.setSize(nextProps.width, nextProps.height, false); | ||
| return false; |
There was a problem hiding this comment.
this function is a bit confusing to me. beside returning should update, it also does a chart.redraw.
but here, a setSize() does not need to be redrawn?
| } | ||
| if (doUpdate) { chart.redraw(false); } | ||
| return false; |
There was a problem hiding this comment.
this return false statement should be moved outside of the if(chart) block.
| }; | ||
|
|
||
|
|
||
| // when more than one histogram is defined use series | ||
| Histogram.propTypes = { | ||
| series: PropTypes.arrayOf(PropTypes.object), // array of objects with data, binColor, and name properties | ||
| xAxis: PropTypes.object, | ||
| yAxis: PropTypes.object, |
There was a problem hiding this comment.
It may be useful if you made this a shape type with defined props. just a thought. I am not sure how useful it is.
| @@ -22,7 +22,7 @@ export const axisParamsShape = PropTypes.shape({ | |||
| label : PropTypes.string, | |||
| unit : PropTypes.string, | |||
| error: PropTypes.string, | |||
| options : PropTypes.string // ex. 'grid,log,flip' | |||
| options : PropTypes.string // ex. 'grid,log,flip,opposite' | |||
|
Regarding the first comment. Highcharts separate redraw and reflow. Reflow
happens automatically on container resize, it's more efficient than redraw.
But you are right that return should not be there, because other props can
change too.
…------------------------------
In src/firefly/js/charts/ui/Histogram.jsx
<#265 (review)>
:
> const chart = this.refs.chart && this.refs.chart.getChart();
if (chart) {
- let doUpdate = false;
- if (height !== nextProps.height || width !== nextProps.width ) {
- chart.setSize(nextProps.width, nextProps.height, false);
- return false;
- }
+ try {
+ let doUpdate = false;
+ if (height !== nextProps.height || width !== nextProps.width ) {
+ chart.setSize(nextProps.width, nextProps.height, false);
+ return false;
this function is a bit confusing to me. beside returning should update, it
also does a chart.redraw.
but here, a setSize() does not need to be redrawn?
|
|
In src/firefly/js/charts/ui/Histogram.jsx <#265 (review)>:
> };
// when more than one histogram is defined use series
Histogram.propTypes = {
series: PropTypes.arrayOf(PropTypes.object), // array of objects with data, binColor, and name properties
+ xAxis: PropTypes.object,
+ yAxis: PropTypes.object,
It may be useful if you made this a shape type with defined props. just a
thought. I am not sure how useful it is.
I was hoping that xAxis and yAxis can hold any relevant Highcharts option.
For now, we pass "reversed" and "opposite", but if you use a component
directly as in
http://localhost:8080/firefly/demo/ffapi-highlevel-charttest.html, you can
supply any option for xAxis or yAxis via these properties.
I am still not sure what would be the best way to supply random Highcharts
properties. If we allow to pass the whole config, how do we map series to
the data? We create series dynamically, based on parameters. In addition
to data, there could be error series, selected points series, a highlight.
How do we give user an ability to tweak options for each of those?
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Separated reversed and opposite ("top" for X, "right" for Y) axis options.
Refactored code to use ColumnOrExpression field in histogram or other charts.
https://jira.lsstcorp.org/browse/DM-8302
To test, open chart options and play with axis options in collapsible for X or Y axes.