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

Fil/grid #992

Closed
wants to merge 6 commits into from
Closed

Fil/grid #992

wants to merge 6 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jul 14, 2022

  • upgrade the grid option to a grid mark
  • fix some cases (band scales)
  • slightly changes the logic for the fx/fy grid option
  • document & test

Note that empty-x has changed visually; it's a case where grid:true is set as a top-level option, but x was defined as {axis:null}. It is now expected (as suggested by @awiederkehr) that we draw the x grid in that case.

includes #987

@Fil Fil requested a review from mbostock July 14, 2022 15:14
src/plot.js Outdated Show resolved Hide resolved
src/plot.js Outdated
Comment on lines 181 to 228
if (options.fy?.grid === true) {
const ty = fy.bandwidth() / 2;
selection.selectAll()
.data(fyDomain.map(d => fy(d) + ty))
.enter()
.append("line")
.attr("y1", d => d)
.attr("y2", d => d)
.attr("x1", dimensions.marginLeft)
.attr("x2", dimensions.width - dimensions.marginRight)
.attr("stroke", "currentColor")
.attr("stroke-opacity", 0.1);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a (non-configurable) alternate path for the grid on the facet axes seems unfortunate. Like, if Plot supports grid on fx and fy, then I’d want to be able to use the Plot.grid mark (or Plot.gridFX? Plot.facetGrid? Plot.frameGrid?) to control how that grid is rendered. I guess that’s not possible because the facet grid needs to extend across facet frames, and thus can’t live in the same space as other marks?

I wonder if there’s a more configurable way that we could do this. One option would be to have a “frame” grid that is repeated for each frame (changing the visual appearance of facet grids). Another option would be to have an array of “facet marks” (name TBD) that are rendered separately below (or above?) the facet frames in a single pass, perhaps where x = fx and y = fy.

I think eventually it’d be nice to figure out how to replace the built-in axes with a custom axis mark, for example…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the use case for the facet grid (it was there so I tried to keep it, and felt it was better to have it outside of axes). I like the idea of having a mark where x would be fx (and y fy); maybe some marks would work out of the box (like Plot.cell) if called in this particular context.

replace the built-in axes with a custom axis mark

Yes, it seems like a natural fast-follow to this PR. It would also help a bit with #147.

Comment on lines +497 to +566
stroke: "currentColor",
strokeOpacity: 0.1
Copy link
Member

@mbostock mbostock Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: something I ran into with the Delaunay mesh mark, but I kind of wanted Plot.grid({stroke: "red"}) to reset the stroke opacity to 1.0 rather than using the default 0.1. I.e., if the stroke is undefined, then the default stroke is currentColor and the default stroke opacity is 0.1, but if the stroke is defined, then the default stroke opacity resets to 1.0. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I also struggled with this — it’s not obvious visually if it’s a stroke width or opacity that makes the line faint, and you have to either inspect or refer to the documentation. I like the suggested change.

mbostock and others added 6 commits July 22, 2022 12:11
fix the grid for ordinal scales
for the fx/fy grids: a small difference is that they're not interrupted anymore (but it seems better this way in fact)
@Fil Fil changed the base branch from mbostock/grid to main July 22, 2022 10:20
@Fil Fil marked this pull request as draft July 22, 2022 10:20
@mbostock mbostock mentioned this pull request Dec 25, 2022
36 tasks
@Fil Fil deleted the fil/grid branch June 7, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants