Add BoxPlot component (Closes #6) #34

Merged
e26chiu merged 9 commits from boxplot-component into main 5 months ago
Collaborator

Done:

  • Display boxplot with left and bottom axis in playground.tsx
  • Add mock data for boxplot
  • Hovering over boxplot displays a Tool Tip box.

My version:
image

Note:

  • No percentage displayed next to value axis labels.

  • Hovering over the individual boxplot does not darken the colour of the boxplot itself.

  • We can add outlier points although it could complicate the graph.

  • Still displays the following error:

    • Error: Hydration failed because the initial UI does not match what was rendered on the server.
    • Error: Text content does not match server-rendered HTML.
    • Error: There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.

    Staging: https://boxplot-component-csc-class-profile-staging-snedadah.k8s.csclub.cloud/

Done: - [x] Display boxplot with left and bottom axis in `playground.tsx` - [x] Add mock data for boxplot - [x] Hovering over boxplot displays a Tool Tip box. My version: ![image](/attachments/6c8c4499-a1bd-4434-9230-7117266691ea) Note: - No percentage displayed next to value axis labels. - Hovering over the individual boxplot does not darken the colour of the boxplot itself. - We can add outlier points although it could complicate the graph. - Still displays the following error: - Error: Hydration failed because the initial UI does not match what was rendered on the server. - Error: Text content does not match server-rendered HTML. - Error: There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering. Staging: https://boxplot-component-csc-class-profile-staging-snedadah.k8s.csclub.cloud/
e26chiu added 4 commits 5 months ago
a258wang was assigned by e26chiu 5 months ago
a258wang removed their assignment 5 months ago
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

Please delete CandlestickChart.tsx since it's empty, and please fix the package-lock.json (it should be sufficient to run npm i and commit the generated package-lock.json).

Otherwise, looks great overall - left a few comments about nits, styling consistency, and a small bug in an edge case 🙂

Please delete `CandlestickChart.tsx` since it's empty, and please fix the `package-lock.json` (it should be sufficient to run `npm i` and commit the generated `package-lock.json`). Otherwise, looks great overall - left a few comments about nits, styling consistency, and a small bug in an edge case 🙂
const DEFAULT_LABEL_SIZE = 16;
interface BoxPlotData {
x: string;
Owner

NIT: maybe name this label or name or something, just to be a bit more descriptive?

NIT: maybe name this `label` or `name` or something, just to be a bit more descriptive?
Poster
Collaborator

The name was left as x since the boxPlot property in the Stats object takes in a field named x. With the changes (of changing x to a more descriptive name), inserting the boxplot data would look like this:

boxPlot: {
      x: data[i].category,
      min: data[i].min,
      firstQuartile: data[i].firstQuartile,
      median: data[i].median,
      thirdQuartile: data[i].thirdQuartile,
      max: data[i].max,
      outliers: [],
},

since the BoxPlotData would have a different label name for x. If we don't implement the changes, inserting boxplot data would look like this:

boxPlot: data[i]

I was wondering whether the name change was really necessary. We could add a comment next to the field x in the interface BoxPlotData to explain its purpose.

The name was left as `x` since the `boxPlot` property in the `Stats` object takes in a field named `x`. With the changes (of changing `x` to a more descriptive name), inserting the boxplot data would look like this: ``` boxPlot: { x: data[i].category, min: data[i].min, firstQuartile: data[i].firstQuartile, median: data[i].median, thirdQuartile: data[i].thirdQuartile, max: data[i].max, outliers: [], }, ``` since the `BoxPlotData` would have a different label name for `x`. If we don't implement the changes, inserting boxplot data would look like this: ``` boxPlot: data[i] ``` I was wondering whether the name change was really necessary. We could add a comment next to the field `x` in the `interface BoxPlotData` to explain its purpose.
Owner

We should be able to do

boxPlot: { ...data[i], x: data[i].category }

which is fairly concise, in my opinion. But ultimately it's up to you - a descriptive JSDoc comment in the interface would be a good idea either way. 🙂

We should be able to do ```typescript boxPlot: { ...data[i], x: data[i].category } ``` which is fairly concise, in my opinion. But ultimately it's up to you - a descriptive JSDoc comment in the interface would be a good idea either way. 🙂
a258wang marked this conversation as resolved
max: number;
firstQuartile: number;
thirdQuartile: number;
outliers: number[];
Owner

NIT: could we make outliers optional?

NIT: could we make `outliers` optional?
a258wang marked this conversation as resolved
outliers: number[];
}
interface TooltipData {
Owner

Consider making the TooltipData type based off of BoxPlotData using Omit and Partial?

https://www.typescriptlang.org/docs/handbook/utility-types.html

Consider making the `TooltipData` type based off of `BoxPlotData` using `Omit` and `Partial`? https://www.typescriptlang.org/docs/handbook/utility-types.html
a258wang marked this conversation as resolved
}
export type StatsPlotProps = {
width: number;
Owner

NIT: add a jsdoc (ie. /** this */) for each prop so people know what each prop does when they're using the component. See the bar graph or word cloud for an example.

NIT: add a jsdoc (ie. `/** this */`) for each prop so people know what each prop does when they're using the component. See the bar graph or word cloud for an example.
a258wang marked this conversation as resolved
left: number;
};
strokeWidth: number;
strokeDashArray: string;
Owner

NIT: I would suggest making strokeWidth and strokeDashArray optional props as well (ie. they already have default values), so people don't need to worry about them unless they want to customize the styling. strokeDashArray in particular needs to be in a specific format, so it'd be great if people didn't have to worry about that when using the component. :)

NIT: I would suggest making `strokeWidth` and `strokeDashArray` optional props as well (ie. they already have default values), so people don't need to worry about them unless they want to customize the styling. `strokeDashArray` in particular needs to be in a specific format, so it'd be great if people didn't have to worry about that when using the component. :)
a258wang marked this conversation as resolved
toolTipFontSize?: number;
};
export default withTooltip<StatsPlotProps, TooltipData>(
Owner

NIT: we usually use non-default exports for components

NIT: we usually use non-default exports for components
a258wang marked this conversation as resolved
const yMax = height - 120;
// formatting data
const d: Stats[] = [];
for (let i = 0; i < data.length; i++) {
Owner

Two nits:

  1. Please name this variable something more descriptive than d. (plotData or formattedData, perhaps?)
  2. Would it work if we did something like this?
const plotData: Stats[] = data.map((d) => { return { boxPlot: d, binData: [] }; })

My personal philosophy (feel free to disagree) when working with React stuff is to prefer functional logic (eg. Array.map) over imperative logic (eg. traditional for-loops) where it makes reasonable sense, since React itself follows more of a functional paradigm.

Two nits: 1. Please name this variable something more descriptive than `d`. (`plotData` or `formattedData`, perhaps?) 2. Would it work if we did something like this? ```typescript const plotData: Stats[] = data.map((d) => { return { boxPlot: d, binData: [] }; }) ``` My personal philosophy (feel free to disagree) when working with React stuff is to prefer functional logic (eg. `Array.map`) over imperative logic (eg. traditional for-loops) where it makes reasonable sense, since React itself follows more of a functional paradigm.
a258wang marked this conversation as resolved
}
// accessors
const x = (d: Stats) => d.boxPlot.x;
Owner

NIT: name accessors as getX, getMin, etc.

NIT: name accessors as `getX`, `getMin`, etc.
a258wang marked this conversation as resolved
// scales
const xScale = scaleBand<string>({
range: [18, xMax - 80], // scaling is needed due to the left offset
Owner

Do these hardcoded numbers still work well for different graph widths, margins, datasets, etc.?

Do these hardcoded numbers still work well for different graph widths, margins, datasets, etc.?
Poster
Collaborator

These hardcoded numbers work well if you change the width, height, margins, and datasets. I haven't tested for whether these hardcoded numbers work when you change the offset values though.

These hardcoded numbers work well if you change the width, height, margins, and datasets. I haven't tested for whether these hardcoded numbers work when you change the offset values though.
});
const boxWidth = xScale.bandwidth();
const constrainedWidth = Math.min(200, boxWidth);
Owner

NIT: since it doesn't look like boxWidth is being used elsewhere, we could do this on one line, ie.

const constrainedBoxWidth = Math.min(xScale.bandwidth(), 200);
NIT: since it doesn't look like `boxWidth` is being used elsewhere, we could do this on one line, ie. ```typescript const constrainedBoxWidth = Math.min(xScale.bandwidth(), 200); ```
a258wang marked this conversation as resolved
const boxWidth = xScale.bandwidth();
const constrainedWidth = Math.min(200, boxWidth);
return width < 10 ? null : (
Owner

NIT: restrictions like these on props are good things to include in the jsdoc descriptions in the props interface. 🙂

NIT: restrictions like these on props are good things to include in the jsdoc descriptions in the props interface. 🙂
Poster
Collaborator

Are you suggesting that I mention this constraint in the StatsPlotProps interface (width >= 10) and I remove this conditional rendering or should I keep this conditional rendering?

Are you suggesting that I mention this constraint in the `StatsPlotProps` interface (width >= 10) and I remove this conditional rendering or should I keep this conditional rendering?
Owner

You can keep the conditional rendering, and then additionally mention it in the interface.

You can keep the conditional rendering, and then additionally mention it in the interface.
a258wang marked this conversation as resolved
const constrainedWidth = Math.min(200, boxWidth);
return width < 10 ? null : (
<div style={{ position: "relative" }}>
Owner

Is this (both the div and the position: relative) necessary?

EDIT: My guess is that the relative positioning is necessary for the tooltip, if so then I would suggest using a className instead of inline styling like this.

Is this (both the `div` and the `position: relative`) necessary? EDIT: My guess is that the relative positioning is necessary for the tooltip, if so then I would suggest using a className instead of inline styling like this.
a258wang marked this conversation as resolved
<svg width={width} height={height}>
<Group top={margin.top} left={margin.left}>
<GridRows
top={10}
Owner

NIT: probably good to pull this out into a constant, seeing as it's (presumably) important for the +10 in the Line components down below to match this value here?

NIT: probably good to pull this out into a constant, seeing as it's (presumably) important for the +10 in the `Line` components down below to match this value here?
a258wang marked this conversation as resolved
<GridColumns
scale={xScale}
width={xMax}
height={yMax + 32}
Owner

I'm curious what the +32 is for? Does this number depend on any other hard-coded values and/or props?

I'm curious what the +32 is for? Does this number depend on any other hard-coded values and/or props?
a258wang marked this conversation as resolved
fill={Color.tertiaryBackground}
to={
new Point({
x: xMax - 20 - strokeWidth,
Owner

Similar to my comment above - does the -20 depend on other hard-coded values and/or user-provided props?

Similar to my comment above - does the -20 depend on other hard-coded values and/or user-provided props?
a258wang marked this conversation as resolved
strokeDasharray={strokeDashArray}
/>
<AxisBottom
top={yMax + 30}
Owner

Same as above - does the +30 depend on other hard-coded values and/or user-provided props?

Same as above - does the +30 depend on other hard-coded values and/or user-provided props?
a258wang marked this conversation as resolved
};
}}
/>
<Group top={6}>
Owner

Same as above - does the 6 depend on other hard-coded values and/or user-provided props?

Same as above - does the 6 depend on other hard-coded values and/or user-provided props?
a258wang marked this conversation as resolved
/>
<Group top={6}>
{d.map((d: Stats, i) => (
<g key={i}>
Owner

NIT: any reason why we're not using the visx Group component here?

NIT: any reason why we're not using the visx `Group` component here?
a258wang marked this conversation as resolved
median={median(d)}
boxWidth={constrainedWidth * 0.4}
fill={Color.primaryAccentLight}
fillOpacity={0.8}
Owner

For consistency with the other graphs, can we set fillOpacity={1}? (or just omit it, since 1 should be the default value)

For consistency with the other graphs, can we set `fillOpacity={1}`? (or just omit it, since `1` should be the default value)
Owner

Also while we're on the topic of styling, we can specify rx={0} and ry={0} to get rid of the rounded corners on the box, which would be consistent with the bar graphs. (Apparently the default value is 2, not 0, which is why we're getting those rounded corners.) 🙂

Also while we're on the topic of styling, we can specify `rx={0}` and `ry={0}` to get rid of the rounded corners on the box, which would be consistent with the bar graphs. (Apparently the default value is 2, not 0, which is why we're getting those rounded corners.) 🙂
a258wang marked this conversation as resolved
minProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(min(d)) ?? 0 + 40,
Owner

NIT: either yScale(min(d))! if we can do a non-null assertion, or simplify 0 + 40 to 40

NIT: either `yScale(min(d))!` if we can do a non-null assertion, or simplify `0 + 40` to `40`
Owner

oh wait I just realized you probably meant (yScale(min(d)) ?? 0) + 40, I think the brackets might be necessary since ?? has a very low precedence when it comes to order of operations.

but comment about ! still applies, if applicable :))

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence

oh wait I just realized you probably meant `(yScale(min(d)) ?? 0) + 40`, I think the brackets might be necessary since ?? has a very low precedence when it comes to order of operations. but comment about ! still applies, if applicable :)) https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
a258wang marked this conversation as resolved
maxProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(max(d)) ?? 0 + 40,
Owner

same as above

same as above
a258wang marked this conversation as resolved
hideTooltip();
},
}}
boxProps={{
Owner

can add stroke: "none" to the boxProps object to get rid of the outline around the box

can add `stroke: "none"` to the `boxProps` object to get rid of the outline around the box
e26chiu marked this conversation as resolved
boxProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(median(d)) ?? 0 + 20,
Owner

same as above

same as above
a258wang marked this conversation as resolved
}}
medianProps={{
style: {
stroke: "white",
Owner

NIT: stroke: Color.label if it works

NIT: `stroke: Color.label` if it works
a258wang marked this conversation as resolved
},
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(median(d)) ?? 0 + 40,
Owner

same as above lol

same as above lol
a258wang marked this conversation as resolved
...defaultTooltipStyles,
backgroundColor: Color.secondaryBackground,
color: Color.primaryText,
padding: "10px",
Owner

NIT: does calc(10rem / 16) work here?

NIT: does `calc(10rem / 16)` work here?
a258wang marked this conversation as resolved
}}
>
<div>
<strong>{tooltipData.name}</strong>
Owner

Can we try to use a more meaningful element here rather than just a div? Maybe a <p>?

Also a NIT: use CSS instead of <strong> if we can

Can we try to use a more meaningful element here rather than just a `div`? Maybe a `<p>`? Also a NIT: use CSS instead of `<strong>` if we can
a258wang marked this conversation as resolved
</div>
<div
style={{
marginTop: "5px",
Owner

NIT: calc(5rem / 16) if it works

Also just a note that we should generally prefer styling via CSS whenever possible, instead of inline styles like this. 🙂

NIT: `calc(5rem / 16)` if it works Also just a note that we should generally prefer styling via CSS whenever possible, instead of inline styles like this. 🙂
a258wang marked this conversation as resolved
{tooltipData.firstQuartile && (
<div>first quartile: {tooltipData.firstQuartile}</div>
)}
{tooltipData.min && <div>min: {tooltipData.min}</div>}
Owner

What happens here if tooltipData.min etc. is 0 or some other falsy (but non-null) value? 😉

This is why we should always prefer the ternary operator for conditional rendering, instead of using the && "hack".

{tooltipData.min ? <div>min: {tooltipData.min}</div> : null}

or in this case, since the value could potentially be 0,

{tooltipData.min != null ? <div>min: {tooltipData.min}</div> : null}

Also, NIT: using a more meaningful html tag like p would be preferred over a div.

What happens here if `tooltipData.min` etc. is `0` or some other falsy (but non-null) value? 😉 This is why we should always prefer the ternary operator for conditional rendering, instead of using the && "hack". ```typescript {tooltipData.min ? <div>min: {tooltipData.min}</div> : null} ``` or in this case, since the value could potentially be `0`, ```typescript {tooltipData.min != null ? <div>min: {tooltipData.min}</div> : null} ``` Also, NIT: using a more meaningful html tag like `p` would be preferred over a `div`.
a258wang marked this conversation as resolved
export default function Home() {
return (
<div className={styles.page}>
<div className={styles.page} suppressHydrationWarning>
Owner

Does this actually do anything? 🤔

Regardless, if you can fix the lint errors and get CI to pass, everything will probably be fine on staging since it's a static site. You should double check which DOM elements are causing the hydration warnings, but my guess is that (a) it's visx's fault and (b) it's only causing wooziness because the dev env tries to use some SSR.

Does this actually do anything? 🤔 Regardless, if you can fix the lint errors and get CI to pass, everything will probably be fine on staging since it's a static site. You should double check which DOM elements are causing the hydration warnings, but my guess is that (a) it's visx's fault and (b) it's only causing wooziness because the dev env tries to use some SSR.
a258wang marked this conversation as resolved
a258wang requested changes 5 months ago
a258wang left a comment
Owner

Oops wait I didn't mean to approve the PR yet - wanted to hear back about some of those comments first

Oops wait I didn't mean to approve the PR yet - wanted to hear back about some of those comments first
Owner

Oh yeah @e26chiu another thing I forgot to mention - would it be possible to give the tooltips a light background with dark text, just to be consistent with the tooltips on other components (eg. word cloud)? Thanks 🙂

Oh yeah @e26chiu another thing I forgot to mention - would it be possible to give the tooltips a light background with dark text, just to be consistent with the tooltips on other components (eg. word cloud)? Thanks 🙂
e26chiu added 1 commit 5 months ago
dd0360e99d refactor + document code
a258wang reviewed 5 months ago
a258wang left a comment
Owner

Thanks for iterating on the feedback, just a few more nits and a small bug!

Also, please fix the package-lock.json by running npm install and committing the generated package-lock.json. (This is blocking approval right now.) Thanks!

Thanks for iterating on the feedback, just a few more nits and a small bug! Also, please fix the `package-lock.json` by running `npm install` and committing the generated `package-lock.json`. (This is blocking approval right now.) Thanks!
.tooltip {
background-color: var(--label);
color: var(--primary-background);
padding: calc(10rem / 16);
Owner

NIT: add border-radius: calc(10rem / 16); to the tooltip to match the tooltip styles on the word cloud more closely. (we can go through the site and fix font size/weight later)

NIT: add `border-radius: calc(10rem / 16);` to the tooltip to match the tooltip styles on the word cloud more closely. (we can go through the site and fix font size/weight later)
a258wang marked this conversation as resolved
.tooltip .toolTipData {
margin-top: 5px;
margin-bottom: 10px;
font-size: 16px;
Owner

NIT: use calc(16rem / 16) etc. for all the px values in this file

NIT: use `calc(16rem / 16)` etc. for all the `px` values in this file
e26chiu marked this conversation as resolved
toolTipLeftOffset,
tooltipData: {
min: getMin(d),
max: getMax(d),
Owner

For the boxProps, you did

tooltipData: { ...d.boxPlot, category: getX(d) }

Would that work for the minProps and maxProps and medianProps as well?

For the `boxProps`, you did ```typescript tooltipData: { ...d.boxPlot, category: getX(d) } ``` Would that work for the `minProps` and `maxProps` and `medianProps` as well?
e26chiu marked this conversation as resolved
hideTooltip();
},
}}
boxProps={{
Owner

NIT: add stroke: none (or something like that) to the boxProps object to get rid of the outline around the box while keeping the whisker lines.

NIT: add `stroke: none` (or something like that) to the boxProps object to get rid of the outline around the box while keeping the whisker lines.
e26chiu marked this conversation as resolved
{tooltipData.firstQuartile ? (
<p>first quartile: {tooltipData.firstQuartile}</p>
) : null}
{tooltipData.min ? <p>min: {tooltipData.min}</p> : null}
Owner

When the minimum (or another one of the stats) is 0, the "min: 0" won't render at all. Should use tooltipData.min != null ? ... for these.

When the minimum (or another one of the stats) is `0`, the "min: 0" won't render at all. Should use `tooltipData.min != null ? ...` for these.
Owner

Though now that everything in the TooltipData type is required, we can probably get rid of all these conditional statements.

Though now that everything in the `TooltipData` type is required, we can probably get rid of all these conditional statements.
e26chiu marked this conversation as resolved
e26chiu added 1 commit 5 months ago
ecbe05530b refactor code + remove stroke on box
e26chiu added 1 commit 5 months ago
a703c60824 fix package-lock + refactor variables
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

🚢

🚢
}
.tooltip .category {
margin: 10px 0 0 0;
Owner

NIT: calc(10rem / 16)

NIT: `calc(10rem / 16)`
e26chiu marked this conversation as resolved
e26chiu added 1 commit 5 months ago
a188a8a3b4 refactor px to rem
e26chiu added 1 commit 5 months ago
e26chiu merged commit 9526f1b0f5 into main 5 months ago
e26chiu deleted branch boxplot-component 5 months ago

Reviewers

a258wang approved these changes 5 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 9526f1b0f5.
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#34
Loading…
There is no content yet.