Plot title additions #2337
Plot title additions #2337
Conversation
🦋 Changeset detectedLatest commit: 35aea89 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| @@ -91,7 +91,7 @@ export const library = [ | |||
| name: "inspect", | |||
| definitions: [ | |||
| makeDefinition([frAny], ([value]) => { | |||
| console.log(value.toString()); | |||
| console.log(value); | |||
There was a problem hiding this comment.
More useful for debugging. toString() really doesn't provide much information.
There was a problem hiding this comment.
Two requests:
- rename file to
PlotTitle(properly capitalized) - move to
src/components? (I don't know if our current fs layout is optimal, but so far we didn't have any components insrc/lib/)
| @@ -117,31 +115,22 @@ const InnerDistributionsChart: FC<{ | |||
| suggestedPadding: { | |||
| left: 10, | |||
| right: 10, | |||
| top: 10 + legendHeight + titleHeight, | |||
| top: 10 + legendHeight, | |||
There was a problem hiding this comment.
There's a slight discrepancy (4px) with the old behavior.
In both versions, canvas is 84px tall:
- 20px bottom padding
- 14px top padding previously (10 + empty titleHeight); 10px now
So previously chart height itself was exactly as was requested, 50px, but it's 54px now.
Of course this doesn't matter much, but to avoid future confusion about what's right, I'd suggest:
- move
suggestedPaddingoutside ofdrawfunction - then, calculate
const heightbased on it,const height = innerHeight + suggestedPadding.top + suggestedPadding.bottom, then we'd be confident that they match and thatinnerHeightis correct
There was a problem hiding this comment.
This is a messy area, thanks for explaining!
For one thing, I imagine we'll later just want the labels to be in HTML, which would change the constraints here.
We'd also want it underneath the samples toolbar, as discussed in my PR description.
| @@ -386,6 +387,7 @@ export type Plot = | |||
| fn: Lambda; | |||
| xScale: Scale; | |||
| yScale: Scale; | |||
| title?: string; | |||
There was a problem hiding this comment.
Was there a problem with also having a title on "relativeValues" plots?
If we had it then you could extract these props into CommonPlotArgs, similar to CommonScaleArgs, and also put get title() accessor in SqAbstractPlot, so it would simplify things a bit.
There was a problem hiding this comment.
Good idea. I just didn't get to adding it to relativeValues plots.
| @@ -56,6 +62,12 @@ export function drawAxes({ | |||
| const tickSize = 2; | |||
|
|
|||
| const padding: Padding = { ...suggestedPadding }; | |||
| if (xAxisTitle) { | |||
| padding.bottom = padding.bottom + 20; | |||
| } | |||
There was a problem hiding this comment.
This is a potential problem, more important than the one about 4px in my other comment.
I'm not proposing any immediate changes here, just flagging a potential technical debt.
By design, drawAxes always had the potential to increase suggested paddings. But previously that didn't happen on vertical padding.
This is a problem because we determine the height of canvas first, then try to draw on it and increase padding, which decreases the chart's main area. If chartHeight in settings is low enough, main area size might become tiny or even negative.
I made this experiment, because AFAIK chartHeight in calculators is the lowest one we use currently: https://squiggle-website-git-plot-additions-quantified-uncertainty.vercel.app/playground?v=dev#code=eNpNTs0KgzAMfpWQ0wTxMNil173AwN3WHaKtKOsPaMoc2nefVdwMId9PPkImVLqhYPjqlUaBd6qMLiy99Ek6gCkNAEVMAh4SG%2B8lPvPNrb0J1g1psepfPFXjxFECzON8lDfjuVDdwKcJEgg4A3u45DCWNRktYIXCdE5Tv4S44%2BRKrAy1EiFmS%2F8vxnznO9vejNJlmOPQ%2BncZrKX%2Bg4L7oOMXsMVItw%3D%3D
Inner height there is still positive, but dangerously close to zero. Ideally, we should always respect inner height (chartHeight) as we the component received and add padding around it.
The circular dependency that needs to be untangled to fix this is a bit complex:
<canvas>needs aheightproperty immediately when the component is mountedheightof the entire canvas isinnerHeight(=chartHeight) + padding that's dynamic and depends on other attributes; sometimes padding also depends on data; right now it's just for horizontal padding (because tick labels can be of arbitrary length), but not for the vertical padding- but that can change in the future too; for example if we decide to output x axis tick labels diagonally when they're too long
- even for
xAxisTitle, you've added 20px here, but the most proper way would be tocontext.measureText
- the problem is,
measureTextrequirescontext, which is obtained throughcanvas.getContext("2d"), but that's too late; so we needheightforcanvas, andcanvas.getContextfor height - in theory, though, any canvas would do for
measureText; we could initialize a single hidden canvas and use it for all measurements
So this is solvable, but would require significant changes or additions to the APIs that we have (useCanvas and drawAxes).
| const fixedFormat = locale.format(".9~f"); | ||
| const siFormat = locale.format(".3~s"); | ||
| const expFormat = locale.format(".3~e"); | ||
| const fixedFormat = locale.format(".3~f"); |
There was a problem hiding this comment.
This causes a blind spot in 0.001..0.00001 interval: https://squiggle-website-git-plot-additions-quantified-uncertainty.vercel.app/playground?v=dev#code=eNqrVkpJTUsszSlxzk9JVbJSMtAzAAJDhZJ8BTDTUElHqTgjvzy4NDc3sahSyaqkqDS1FgC78hDF
This is a problem particularly for relative values where dists like this are going to be frequent.
It's especially confusing if you try to move your cursor from 0 to some value around 0.001:
- first you get tiny non-zero values (1e-6)
- then you get to this blind spot where verything is 0
- and then from 0.001 it's non-zero again
.3~r works well for values under 0.001:
> d3.format('0.3~f')(0.03123123)
'0.031'
> d3.format('0.3~f')(0.0003123123)
'0'
> d3.format('0.3~r')(0.03123123)
'0.0312'
> d3.format('0.3~r')(0.0003123123)
'0.000312'
Then above 1000 it becomes bad again:
> d3.format('0.3~r')(12345.123145125)
'12300'
How about another else if branch below that uses .3~r for values below 0.001 and .3~f above that?
It's unfortunate that there's nothing in d3 for "show all digits before decimal point, and first N significant digits after that".
There was a problem hiding this comment.
Good idea about the else if branch, will try.
I was nervous about this, but was figuring I'd address later, but happy to do it now.
There was a problem hiding this comment.
I just reverted this for this story, will move to a separate story.
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2337 +/- ##
==========================================
+ Coverage 71.69% 71.74% +0.05%
==========================================
Files 111 111
Lines 5670 5688 +18
Branches 1111 1120 +9
==========================================
+ Hits 4065 4081 +16
- Misses 1597 1599 +2
Partials 8 8 ☔ View full report in Codecov by Sentry. |
|
Closed #1725 |
This PR makes a few additions to plots:
titleattributes, but the other plot types didn't. Now, they all do.titleattribute. This allows you to specify a title for the xAxis and yAxis. This is done in D3.tickFormats in squiggle-language, using a regex.inspect()now outputs the JS object to the console, instead of its toString().One minor issue (shown here), is that it might be better if the xScaleTitle was shown under the samples bar. Or maybe, we should move the samples bar to be over the x-axis, overlapping with the distribution. I'm not sure.