Gird #712
Gird #712
Conversation
…sses scales only at the bottom level of nested splits.
… translation to group-by
…uld be present in fixtures in unit tests
| const { dataCube: { attributes, source, options: { customAggregations, customTransforms } } } = essence; | ||
| const query = makeQuery(essence, timekeeper); | ||
| const { visualization, dataCube: { attributes, source, options: { customAggregations, customTransforms } } } = essence; | ||
| const queryFn = visualization.name === "grid" ? gridQuery : standardQuery; |
There was a problem hiding this comment.
leaking abstraction, visualisation is responsible for "queryfn" so could be move this logic into visualisation itself?
There was a problem hiding this comment.
Yes, very leaking. These components are very far apart and don't have a good point to communicate. I knowingly made this change in the name of technical debt :)
There was a problem hiding this comment.
sure, just make a note and we will discuss how to define visualisation-make-query abstraction later on
| @@ -109,8 +109,12 @@ export class BaseVisualization<S extends BaseVisualizationState> extends React.C | |||
| return this.debouncedCallExecutor(essence, timekeeper); | |||
| } | |||
|
|
|||
| protected getQuery(essence: Essence, timekeeper: Timekeeper): Expression { | |||
There was a problem hiding this comment.
why this function only delegates to another function?
There was a problem hiding this comment.
This is base/default implementation. Specific Visualisations can override this.
And logic is extracted to function for better maintainability.
There was a problem hiding this comment.
clear, w need better abstraction for this in the future, just make a note for further work
|
|
||
| setSegmentWidth = (segmentWidth: number) => this.setState({ segmentWidth }); | ||
|
|
||
| private getIdealColumnWidth(): number { |
There was a problem hiding this comment.
How to test calculation if method is private? I would extract layout math form the component class.
There was a problem hiding this comment.
Probably you're right. As always, these functions mostly connect multiple variables from object state so tests will get messy. But extraction is good first step!
| @@ -50,7 +49,6 @@ export const MeasureRow: React.SFC<MeasureRowProps> = props => { | |||
| highlight={highlight} | |||
| scale={scales[i]} | |||
| cellWidth={cellWidth} | |||
| lastLevel={datum["__nest"] === splitLength} | |||
There was a problem hiding this comment.
No, this logic is extracted higher in component tree and better named.
| split.changeLimit(100).changeSort(sort))); | ||
|
|
||
| if (splits.equals(newSplits)) { | ||
| return Resolve.ready(isSelectedVisualization ? 10 : 6); |
There was a problem hiding this comment.
Magic numbers, do we have constants for that?
There was a problem hiding this comment.
We don't and honestly I don't know what specific numbers mean. I think we need to heavily rework this mechanism in future.
…o we can move forward before revamping whole SplitMenu component!
| @@ -40,7 +40,9 @@ function applySeries(series: List<ConcreteSeries>, timeShiftEnv: TimeShiftEnv, n | |||
| } | |||
|
|
|||
| function applyLimit({ limit }: Split) { | |||
| const limitExpression = new LimitExpression({ value: limit }); | |||
| // TODO: this calculation is for evaluation purpose. We should add custom split values for Grid and remove this multiplication! | |||
| const value = limit * 10; | |||
No description provided.