-
Notifications
You must be signed in to change notification settings - Fork 14
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In css, it would be nothing more than:
.bar-type .domain {
display: none;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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