switch to scale band for bars #17
Conversation
| @@ -161,10 +162,14 @@ export default function Axis (_container) { | |||
| } | |||
|
|
|||
| function drawAxis () { | |||
| cache.root.select(".axis.x") | |||
| const xAxis = cache.root.select(".axis.x") | |||
There was a problem hiding this comment.
Wouldn't be easier to simply hide the .domain for these chart types? It would also avoid adding chartType as a dependency for the axis component and styling logic in the render logic.
There was a problem hiding this comment.
where would the logic for hiding it live? how would that work? IMO it makes sense to do it here as it's where the axises are being constructed.
I suppose somewhere in bar.js there could be some logic that selects the x axis's path.domain and hides / removes it. But it feels odd to have it there.
Or in a SCSS partial for the bar component there could be some name-spacing that allows for hiding it, but that might get ugly with using lots of selectors.
IMO it seems more semantic to do it in the axis component as that's where the axises are being rendered.
There was a problem hiding this comment.
In css, it would be nothing more than:
.bar-type .domain {
display: none;
}
There was a problem hiding this comment.
are you sure that would override D3's inline styles?
|
|
||
| if (config.keyType === "time") { | ||
| xScale = d3.scaleTime() | ||
| } else if (config.keyType === "number") { | ||
| xScale = d3.scaleLinear() | ||
| } else if (config.keyType === "string" && isBarChart) { |
There was a problem hiding this comment.
I need to test how this logic handle the case where chartType is for example ["bar", "bar", "bar"] (as can be the case in combo chart) instead of type "bar". Also, this will probably break when using stackedBar with time and number keyTypes, which is not a requirement in Immerse right now, but is already available in mapd3 and may become a requirement when we will replace the histogram for example.
There was a problem hiding this comment.
Makes sense to think ahead on this one. The more "d3 way" of doing it seems to be using two ordinal scales, one for the grouping and one for the group category: https://bl.ocks.org/mbostock/3887051
Benefit seems to be that it makes adjusting the padding between bars and groups more declarative. It might also be more familiar to other d3 devs who come across this code.
There was a problem hiding this comment.
I agree. But we do have numerical and time axis on group bar chart (combo) already, so that may break.
There was a problem hiding this comment.
I'm fairly certain that you can treat numeric and temporal values as ordinal with d3.scaleBand, but would need to double check.
|
|
||
| if (config.keyType === "time") { | ||
| xScale = d3.scaleTime() | ||
| } else if (config.keyType === "number") { | ||
| xScale = d3.scaleLinear() | ||
| } else if (config.keyType === "string" && isBarChart) { | ||
| xScale = d3.scaleBand() |
There was a problem hiding this comment.
I like the idea of using scaleBand for bar chart, which is more typical d3. Let's test the side-effects together and evaluate the alternative I will provide in another PR. Then we can decide which solution to adopt.
There was a problem hiding this comment.
I think it makes total sense to let D3 do the work when possible, rather than doing it manually oneself.
There was a problem hiding this comment.
I'm just worried about side-effects, for example with matching line scalePoint with bar scaleBand, when doing numerical with stacked, etc. We can try both and see which one as less side-effects, and revisit later for polishing. I want us to keep some time after 3.6 to polish the architecture and think about developer experience.
|
As discussed, closing for now. |
WIP, addresses #16