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.

Sign up for GitHub

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

Jump to bottom

Plot title additions #2337

Merged
merged 10 commits into from
Oct 17, 2023
Merged

Plot title additions #2337

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
81ba7f9
Add check to scale tick format
OAGr Oct 15, 2023
5933ada
Added title to most plots, and xAxisTitle and yAxisTitle to relevant …
OAGr Oct 15, 2023
23edd85
Adjusted color and font of Axis titles
OAGr Oct 16, 2023
e87eb2a
Small fixes, use custom tickFormat for SummaryTable
OAGr Oct 16, 2023
c46b6d4
Added changeset, fixed tests, minor cleaning
OAGr Oct 16, 2023
273d70a
Adding back modified frDict definition, for tests to work
OAGr Oct 16, 2023
789a4a0
Adjusted xAxisTitle to change position based on padding.bottom
OAGr Oct 16, 2023
9659652
Implemented changes requested in CR
OAGr Oct 16, 2023
5f0d434
Minor quickfix
OAGr Oct 16, 2023
35aea89
Fixed tests
OAGr Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 8 additions & 17 deletions packages/components/src/components/DistributionsChart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,11 @@ const InnerDistributionsChart: FC<{
const legendItemHeight = 16;
const sampleBarHeight = 5;

const showTitle = !!plot.title;
const titleHeight = showTitle ? 20 : 4;
const legendHeight = isMulti ? legendItemHeight * shapes.length : 0;
const _showSamplesBar = showSamplesBar && samples.length;
const samplesFooterHeight = _showSamplesBar ? 10 : 0;

const height =
innerHeight + legendHeight + titleHeight + samplesFooterHeight + 30;
const height = innerHeight + legendHeight + samplesFooterHeight + 34;

const { xScale, yScale } = useMemo(() => {
const xScale = sqScaleToD3(plot.xScale);
Expand Down Expand Up @@ -117,7 +114,7 @@ const InnerDistributionsChart: FC<{
suggestedPadding: {
left: 10,
right: 10,
top: 10 + legendHeight + titleHeight,
top: 10 + legendHeight,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a slight discrepancy (4px) with the old behavior.

Before: https://www.squiggle-language.com/playground?v=dev#code=eNqrVkpJTUsszSlxzk9JVbJSqlCwVQiuzNUrzctMyy%2FK1TDSMTTQVKoFAAjgDIc%3D

After: https://squiggle-website-git-plot-additions-quantified-uncertainty.vercel.app/playground?v=dev#code=eNqrVkpJTUsszSlxzk9JVbJSqlCwVQiuzNUrzctMyy%2FK1TDSMTTQVKoFAAjgDIc%3D

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 suggestedPadding outside of draw function
  • then, calculate const height based on it, const height = innerHeight + suggestedPadding.top + suggestedPadding.bottom, then we'd be confident that they match and that innerHeight is correct

Copy link
Contributor Author

@OAGr OAGr Oct 16, 2023 •
edited

Choose a reason for hiding this comment

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

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.

bottom: 20 + samplesFooterHeight,
},
xScale,
Expand All @@ -127,21 +124,11 @@ const InnerDistributionsChart: FC<{
xTickFormat: plot.xScale.tickFormat,
});

if (plot.title) {
context.save();
context.textAlign = "center";
context.textBaseline = "top";
context.fillStyle = "black";
context.font = "bold 12px sans-serif";
context.fillText(plot.title, width / 2, 4);
context.restore();
}

if (isMulti) {
const radius = 5;
for (let i = 0; i < shapes.length; i++) {
context.save();
context.translate(padding.left, titleHeight + legendItemHeight * i);
context.translate(padding.left, legendItemHeight * i);
context.fillStyle = getColor(i);
drawCircle({
context,
Expand Down Expand Up @@ -266,7 +253,6 @@ const InnerDistributionsChart: FC<{
[
height,
legendHeight,
titleHeight,
samplesFooterHeight,
shapes,
samples,
Expand Down Expand Up @@ -407,6 +393,11 @@ export const DistributionsChart: FC<DistributionsChartProps> = ({

return (
<div className="flex flex-col items-stretch">
{plot.title && (
<div className="text-center font-semibold text-slate-600 text-sm">
{plot.title}
</div>
)}
{plot.xScale.tag === "log" && shapes.value.some(hasMassBelowZero) ? (
<ErrorAlert heading="Log Domain Error">
Cannot graph distribution with negative values on logarithmic scale.
Expand Down
19 changes: 11 additions & 8 deletions packages/components/src/components/DynamicSquiggleViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { getResultVariables, getResultValue } from "../lib/utility.js";
import { CodeEditorHandle } from "./CodeEditor.js";
import { PartialPlaygroundSettings } from "./PlaygroundSettings.js";
import { SquiggleViewerHandle } from "./SquiggleViewer/index.js";
import { ErrorBoundary } from "./ErrorBoundary.js";

type Props = {
squiggleOutput: SquiggleOutput | undefined;
Expand Down Expand Up @@ -34,14 +35,16 @@ export const DynamicSquiggleViewer = forwardRef<SquiggleViewerHandle, Props>(
// `opacity-0 squiggle-semi-appear` would be better, but won't work reliably until we move Squiggle evaluation to Web Workers
<div className="absolute z-10 inset-0 bg-white opacity-50" />
)}
<SquiggleViewer
{...settings}
ref={viewerRef}
localSettingsEnabled={localSettingsEnabled}
resultVariables={getResultVariables(squiggleOutput)}
resultItem={getResultValue(squiggleOutput)}
editor={editor}
/>
<ErrorBoundary>
<SquiggleViewer
{...settings}
ref={viewerRef}
localSettingsEnabled={localSettingsEnabled}
resultVariables={getResultVariables(squiggleOutput)}
resultItem={getResultValue(squiggleOutput)}
editor={editor}
/>
</ErrorBoundary>
</div>
) : null;

Expand Down
32 changes: 32 additions & 0 deletions packages/components/src/components/ErrorBoundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
"use client";

import { Component, PropsWithChildren } from "react";

type State = {
error?: Error;
};

export class ErrorBoundary extends Component<PropsWithChildren, State> {
public state: State = {};

public static getDerivedStateFromError(error: Error): State {
return { error };
}

componentDidCatch() {}

public render() {
if (this.state.error) {
return (
<div className="m-2 p-4 bg-red-300 rounded">
<header className="mb-2 font-semibold">Fatal Error</header>
<div className="mb-2">{this.state.error.message}</div>
<div className="mb-2">Try reloading the browser.</div>
<pre className="text-xs overflow-auto">{this.state.error.stack}</pre>
</div>
);
}

return this.props.children;
}
}
8 changes: 8 additions & 0 deletions packages/components/src/components/FunctionChart/DistFunctionChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ function useDrawDistFunctionChart({
height,
context,
xTickFormat: plot.xScale.tickFormat,
yTickFormat: plot.yScale.tickFormat,
xAxisTitle: plot.xScale.title,
yAxisTitle: plot.yScale.title,
});
d3ref.current = { frame, xScale };

Expand Down Expand Up @@ -303,6 +306,11 @@ export const DistFunctionChart: FC<FunctionChart1DistProps> = ({

return (
<div className="flex flex-col items-stretch">
{plot.title && (
<div className="text-center font-semibold text-slate-600 text-sm">
{plot.title}
</div>
)}
<div ref={refs.setReference}>
<canvas ref={canvasRef} className={canvasClasses}>
Chart for {plot.toString()}
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/components/FunctionChart/NumericFunctionChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ export const NumericFunctionChart: FC<Props> = ({
yScale,
xTickFormat: plot.xScale.tickFormat,
yTickFormat: plot.yScale.tickFormat,
xAxisTitle: plot.xScale.title,
yAxisTitle: plot.yScale.title,
});

if (
Expand Down Expand Up @@ -119,6 +121,11 @@ export const NumericFunctionChart: FC<Props> = ({

return (
<div className="flex flex-col items-stretch">
{plot.title && (
<div className="text-center font-semibold text-slate-600 text-sm">
{plot.title}
</div>
)}
<canvas ref={ref} className={canvasClasses}>
Chart for {plot.toString()}
</canvas>
Expand Down
13 changes: 8 additions & 5 deletions packages/components/src/components/FunctionChart/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
import { SquiggleErrorAlert } from "../SquiggleErrorAlert.js";
import { DistFunctionChart } from "./DistFunctionChart.js";
import { NumericFunctionChart } from "./NumericFunctionChart.js";
import { ErrorBoundary } from "../ErrorBoundary.js";

type FunctionChartProps = {
fn: SqLambda;
Expand Down Expand Up @@ -113,11 +114,13 @@ export const FunctionChart: FC<FunctionChartProps> = ({
});

return (
<NumericFunctionChart
plot={plot}
environment={environment}
height={height}
/>
<ErrorBoundary>
<NumericFunctionChart
plot={plot}
environment={environment}
height={height}
/>
</ErrorBoundary>
);
}
default:
Expand Down
71 changes: 37 additions & 34 deletions packages/components/src/components/SquiggleViewer/VariableBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
pathToShortName,
} from "./utils.js";
import { useEffectRef } from "../../lib/hooks/useEffectRef.js";
import { ErrorBoundary } from "../ErrorBoundary.js";

type SettingsMenuParams = {
// Used to notify VariableBox that settings have changed, so that VariableBox could re-render itself.
Expand Down Expand Up @@ -234,40 +235,42 @@ export const VariableBox: FC<VariableBoxProps> = ({
);

return (
<div ref={saveRef}>
{(name !== undefined || isRoot) && (
<header
className={clsx(
"flex justify-between group",
isFocused ? "mb-2" : "hover:bg-stone-100 rounded-md"
)}
>
<div className="inline-flex items-center">
{!isFocused && triangleToggle()}
{headerName}
{!isFocused && headerPreview()}
{!isFocused && !isOpen && commentIcon()}
{!isRoot && editor && headerFindInEditorButton()}
</div>
<div className="inline-flex space-x-1">
{isOpen && headerString()}
{isOpen && headerSettingsButton()}
</div>
</header>
)}
{isOpen && (
<div className="flex w-full pt-1">
{!isFocused && isDictOrList && leftCollapseBorder()}
{!isFocused && !isDictOrList && !isRoot && (
<div className="flex w-4 min-w-[1rem]" /> // min-w-1rem = w-4
)}
<div className="grow">
{commentPosition === "top" && hasComment && showComment()}
{children(getAdjustedMergedSettings(path))}
{commentPosition === "bottom" && hasComment && showComment()}
<ErrorBoundary>
<div ref={saveRef}>
{(name !== undefined || isRoot) && (
<header
className={clsx(
"flex justify-between group",
isFocused ? "mb-2" : "hover:bg-stone-100 rounded-md"
)}
>
<div className="inline-flex items-center">
{!isFocused && triangleToggle()}
{headerName}
{!isFocused && headerPreview()}
{!isFocused && !isOpen && commentIcon()}
{!isRoot && editor && headerFindInEditorButton()}
</div>
<div className="inline-flex space-x-1">
{isOpen && headerString()}
{isOpen && headerSettingsButton()}
</div>
</header>
)}
{isOpen && (
<div className="flex w-full pt-1">
{!isFocused && isDictOrList && leftCollapseBorder()}
{!isFocused && !isDictOrList && !isRoot && (
<div className="flex w-4 min-w-[1rem]" /> // min-w-1rem = w-4
)}
<div className="grow">
{commentPosition === "top" && hasComment && showComment()}
{children(getAdjustedMergedSettings(path))}
{commentPosition === "bottom" && hasComment && showComment()}
</div>
</div>
</div>
)}
</div>
)}
</div>
</ErrorBoundary>
);
};
31 changes: 31 additions & 0 deletions packages/components/src/lib/draw/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ interface DrawAxesParams {
yTickCount?: number;
xTickFormat?: string;
yTickFormat?: string;
xAxisTitle?: string;
yAxisTitle?: string;
}

export function drawAxes({
Expand All @@ -46,6 +48,8 @@ export function drawAxes({
yTickCount = Math.max(Math.min(Math.floor(height / 100), 12), 3),
xTickFormat: xTickFormatSpecifier = defaultTickFormatSpecifier,
yTickFormat: yTickFormatSpecifier = defaultTickFormatSpecifier,
xAxisTitle,
yAxisTitle,
}: DrawAxesParams) {
const xTicks = xScale.ticks(xTickCount);
const xTickFormat = xScale.tickFormat(xTickCount, xTickFormatSpecifier);
Expand All @@ -56,6 +60,12 @@ export function drawAxes({
const tickSize = 2;

const padding: Padding = { ...suggestedPadding };
if (xAxisTitle) {
padding.bottom = padding.bottom + 20;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 a height property immediately when the component is mounted
  • height of the entire canvas is innerHeight (=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 to context.measureText
  • the problem is, measureText requires context, which is obtained through canvas.getContext("2d"), but that's too late; so we need height for canvas, and canvas.getContext for 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).

if (yAxisTitle) {
padding.left = padding.left + 35;
}

// measure tick sizes for dynamic padding
if (!hideYAxis) {
Expand All @@ -70,6 +80,27 @@ export function drawAxes({
});
}

if (xAxisTitle) {
const titleX = width / 2; // center the title
const titleY = height - 8; // adjust this value based on desired distance from x-axis
context.textAlign = "center";
context.textBaseline = "bottom";
context.font = "bold 12px Arial";
context.fillText(xAxisTitle, titleX, titleY);
}
if (yAxisTitle) {
const titleY = height / 2; // center the title vertically
const titleX = 0;
context.save(); // save the current context state
context.translate(titleX, titleY);
context.rotate(-Math.PI / 2); // rotate 90 degrees counter-clockwise
context.textAlign = "center";
context.textBaseline = "top";
context.font = "bold 12px Arial"; // adjust font size and style as needed
context.fillText(yAxisTitle, 0, 0);
context.restore(); // restore the context state to before rotation and translation
}

const frame = new CartesianFrame({
context,
x0: padding.left,
Expand Down
7 changes: 4 additions & 3 deletions packages/components/src/stories/SquiggleChart/Distributions.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,15 @@ export const ContinuousSampleSet1MSamples: Story = {

export const Discrete: Story = {
args: {
code: "mx(0, 1, 3, 5, 8, 10, [0.1, 0.8, 0.5, 0.3, 0.2, 0.1])",
code: "mx([0, 1, 3, 5, 8, 10], [0.1, 0.8, 0.5, 0.3, 0.2, 0.1])",
},
};

export const Scales: Story = {
name: "Continuous with scales",
args: {
code: `Plot.dist({
dist: -1 to 5,
dist: 1 to 5,
xScale: Scale.symlog(),
yScale: Scale.power({ exponent: 0.1 }),
})`,
Expand All @@ -81,7 +81,7 @@ export const CustomTickFormat: Story = {
export const Mixed: Story = {
name: "Mixed",
args: {
code: "mx(0, 1, 3, 5, 8, normal(8, 1), [0.1, 0.3, 0.4, 0.35, 0.2, 0.8])",
code: "mx([0, 1, 3, 5, 8, normal(8, 1)], [0.1, 0.3, 0.4, 0.35, 0.2, 0.8])",
},
};

Expand All @@ -90,6 +90,7 @@ export const MultiplePlots: Story = {
args: {
code: `
Plot.dists({
title: "Multiple plots",
dists: [
{
name: "one",
Expand Down