Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Add size setting for plot lines #7013

Merged

Conversation

alexern14
Copy link
Contributor

User-Facing Changes

Add line size setting option to plot panel.

Description

The following image shows the blue series with the auto line size which is unchanged, and the orange series with an increased line size.
image

@defunctzombie defunctzombie added the linear Adding this label will copy the issue to Linear label Oct 23, 2023
@jtbandes
Copy link
Member

Internal tracking issue: FG-5377

@alexern14
Copy link
Contributor Author

@jtbandes @cfoust Is there any update on this?

@cfoust
Copy link
Contributor

cfoust commented Nov 1, 2023

Looks good to me! Might wanna try rebasing and pushing again to see if that test error will disappear

@@ -58,6 +58,14 @@ const makeSeriesNode = memoizeWeak(
label: t("color"),
value: path.color ?? lineColors[index % lineColors.length],
},
size: {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename this (and the settings key) to lineSize? Seems better to be a bit more explicit for future sanity.

@@ -58,6 +58,14 @@ const makeSeriesNode = memoizeWeak(
label: t("color"),
value: path.color ?? lineColors[index % lineColors.length],
},
size: {
input: "number",
label: "Line Size",
Copy link
Member

Choose a reason for hiding this comment

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

This should be t("lineSize") and added to i18n/en/plot.ts so it can be localized. Also we generally use sentence case, i.e. "Line size" instead of "Line Size".

@alexern14 alexern14 force-pushed the feature/add-plot-line-size-setting branch from cd599b2 to bd6f01a Compare November 1, 2023 19:29
@alexern14
Copy link
Contributor Author

alexern14 commented Nov 1, 2023

@jtbandes This is my first time putting up a PR for you guys. What's the process for getting the UI reviewed, tests approved, and Vercel authorized?

@jtbandes
Copy link
Member

jtbandes commented Nov 1, 2023

We can take care of the approval/merging step. Could you also add another entry to Plot/index.stories.tsx to create a new screenshot test demonstrating the line size functionality? You should be able to copy/paste one of the simpler entries there (e.g. HiddenConnectingLines) and just add a lineSize value to the hard-coded config.

Comment on lines 1688 to 1690
showLine: false,
timestampMethod: "receiveTime",
lineSize: 1.4,
Copy link
Member

@jtbandes jtbandes Nov 1, 2023

Choose a reason for hiding this comment

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

Making the difference more obvious since it currently doesn't really show up in the screenshot: https://www.chromatic.com/test?appId=603ec8bf7908b500231841e2&id=6542cd42f98cef426c29a020

Suggested change
showLine: false,
timestampMethod: "receiveTime",
lineSize: 1.4,
timestampMethod: "receiveTime",
lineSize: 2.5,

@alexern14 alexern14 force-pushed the feature/add-plot-line-size-setting branch from 5a19037 to 91628e3 Compare November 2, 2023 17:41
{
value: "/some_topic/location.pose.velocity",
enabled: true,
showLine: false,
Copy link
Member

Choose a reason for hiding this comment

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

Please remove the showLine: false too since otherwise we see only the points in the screenshot. The new 2.5 size looks good though! They're obviously bigger than the other points

@alexern14 alexern14 force-pushed the feature/add-plot-line-size-setting branch from 91628e3 to 92726cf Compare November 2, 2023 19:08
@jtbandes jtbandes merged commit 6927296 into foxglove:main Nov 2, 2023
12 of 14 checks passed
@jtbandes
Copy link
Member

jtbandes commented Nov 2, 2023

Thank you!

@alexern14 alexern14 deleted the feature/add-plot-line-size-setting branch November 2, 2023 20:25
jhurliman referenced this pull request in mvi-llc/sviz Mar 5, 2024
**User-Facing Changes**
Add line size setting option to plot panel.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linear Adding this label will copy the issue to Linear
Development

Successfully merging this pull request may close these issues.

None yet

4 participants