www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

Add BoxPlot component (Closes #6) #34

Merged
e26chiu merged 9 commits from boxplot-component into main 2022-09-03 11:13:59 -04:00
Conversation 1 Commits 9 Files Changed 6 +519 -1
e26chiu commented 2022-08-18 17:12:23 -04:00
Contributor
Copy Link

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/
image.png
25 KiB
e26chiu added 4 commits 2022-08-18 17:12:24 -04:00
continuous-integration/drone/push Build is failing Details
2496e34798 add boxplot
a258wang was assigned by e26chiu 2022-08-18 17:12:38 -04:00
a258wang removed their assignment 2022-08-24 21:48:10 -04:00
a258wang approved these changes 2022-08-25 16:42:02 -04:00
a258wang left a comment
Owner
Copy Link

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 🙂
components/Boxplot.tsx
@ -0,0 +20,4 @@
const DEFAULT_LABEL_SIZE = 16;
interface BoxPlotData {
x: string;
a258wang commented 2022-08-25 15:28:16 -04:00
Owner
Copy Link

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?
e26chiu commented 2022-08-27 11:20:03 -04:00
Author
Contributor
Copy Link

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.
a258wang commented 2022-08-27 21:53:41 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +26,4 @@
max: number;
firstQuartile: number;
thirdQuartile: number;
outliers: number[];
a258wang commented 2022-08-25 16:35:57 -04:00
Owner
Copy Link

NIT: could we make outliers optional?

NIT: could we make `outliers` optional?
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +29,4 @@
outliers: number[];
}
interface TooltipData {
a258wang commented 2022-08-25 15:31:02 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +39,4 @@
}
export type StatsPlotProps = {
width: number;
a258wang commented 2022-08-25 15:32:22 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +47,4 @@
left: number;
};
strokeWidth: number;
strokeDashArray: string;
a258wang commented 2022-08-25 15:35:40 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +58,4 @@
toolTipFontSize?: number;
};
export default withTooltip<StatsPlotProps, TooltipData>(
a258wang commented 2022-08-25 15:36:00 -04:00
Owner
Copy Link

NIT: we usually use non-default exports for components

NIT: we usually use non-default exports for components
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +86,4 @@
const yMax = height - 120;
// formatting data
const d: Stats[] = [];
for (let i = 0; i < data.length; i++) {
a258wang commented 2022-08-25 15:45:32 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +94,4 @@
}
// accessors
const x = (d: Stats) => d.boxPlot.x;
a258wang commented 2022-08-25 15:46:26 -04:00
Owner
Copy Link

NIT: name accessors as getX, getMin, etc.

NIT: name accessors as `getX`, `getMin`, etc.
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +103,4 @@
// scales
const xScale = scaleBand<string>({
range: [18, xMax - 80], // scaling is needed due to the left offset
a258wang commented 2022-08-25 15:47:05 -04:00
Owner
Copy Link

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.?
e26chiu commented 2022-08-28 16:04:43 -04:00
Author
Contributor
Copy Link

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.
👍 1
components/Boxplot.tsx Outdated
@ -0,0 +123,4 @@
});
const boxWidth = xScale.bandwidth();
const constrainedWidth = Math.min(200, boxWidth);
a258wang commented 2022-08-25 15:49:29 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +125,4 @@
const boxWidth = xScale.bandwidth();
const constrainedWidth = Math.min(200, boxWidth);
return width < 10 ? null : (
a258wang commented 2022-08-25 15:50:55 -04:00
Owner
Copy Link

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. 🙂
e26chiu commented 2022-08-28 13:43:26 -04:00
Author
Contributor
Copy Link

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?
a258wang commented 2022-08-28 15:36:00 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +126,4 @@
const constrainedWidth = Math.min(200, boxWidth);
return width < 10 ? null : (
<div style={{ position: "relative" }}>
a258wang commented 2022-08-25 15:51:16 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +130,4 @@
<svg width={width} height={height}>
<Group top={margin.top} left={margin.left}>
<GridRows
top={10}
a258wang commented 2022-08-25 15:53:46 -04:00
Owner
Copy Link

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
components/Boxplot.tsx
@ -0,0 +143,4 @@
<GridColumns
scale={xScale}
width={xMax}
height={yMax + 32}
a258wang commented 2022-08-25 15:56:22 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +162,4 @@
fill={Color.tertiaryBackground}
to={
new Point({
x: xMax - 20 - strokeWidth,
a258wang commented 2022-08-25 15:57:49 -04:00
Owner
Copy Link

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
components/Boxplot.tsx
@ -0,0 +172,4 @@
strokeDasharray={strokeDashArray}
/>
<AxisBottom
top={yMax + 30}
a258wang commented 2022-08-25 15:58:14 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +203,4 @@
};
}}
/>
<Group top={6}>
a258wang commented 2022-08-25 15:59:09 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +205,4 @@
/>
<Group top={6}>
{d.map((d: Stats, i) => (
<g key={i}>
a258wang commented 2022-08-25 15:59:53 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +220,4 @@
median={median(d)}
boxWidth={constrainedWidth * 0.4}
fill={Color.primaryAccentLight}
fillOpacity={0.8}
a258wang commented 2022-08-25 16:11:50 -04:00
Owner
Copy Link

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)
a258wang commented 2022-08-25 16:21:40 -04:00
Owner
Copy Link

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
components/Boxplot.tsx
@ -0,0 +227,4 @@
minProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(min(d)) ?? 0 + 40,
a258wang commented 2022-08-25 16:13:38 -04:00
Owner
Copy Link

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`
a258wang commented 2022-08-25 16:17:35 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +242,4 @@
maxProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(max(d)) ?? 0 + 40,
a258wang commented 2022-08-25 16:14:04 -04:00
Owner
Copy Link

same as above

same as above
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +254,4 @@
hideTooltip();
},
}}
boxProps={{
a258wang commented 2022-08-25 16:19:46 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +257,4 @@
boxProps={{
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(median(d)) ?? 0 + 20,
a258wang commented 2022-08-25 16:17:52 -04:00
Owner
Copy Link

same as above

same as above
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +271,4 @@
}}
medianProps={{
style: {
stroke: "white",
a258wang commented 2022-08-25 16:22:33 -04:00
Owner
Copy Link

NIT: stroke: Color.label if it works

NIT: `stroke: Color.label` if it works
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +275,4 @@
},
onMouseOver: () => {
showTooltip({
tooltipTop: yScale(median(d)) ?? 0 + 40,
a258wang commented 2022-08-25 16:22:43 -04:00
Owner
Copy Link

same as above lol

same as above lol
a258wang marked this conversation as resolved
components/Boxplot.tsx
@ -0,0 +302,4 @@
...defaultTooltipStyles,
backgroundColor: Color.secondaryBackground,
color: Color.primaryText,
padding: "10px",
a258wang commented 2022-08-25 16:23:09 -04:00
Owner
Copy Link

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

NIT: does `calc(10rem / 16)` work here?
a258wang marked this conversation as resolved
components/Boxplot.tsx Outdated
@ -0,0 +306,4 @@
}}
>
<div>
<strong>{tooltipData.name}</strong>
a258wang commented 2022-08-25 16:24:55 -04:00
Owner
Copy Link

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
components/Boxplot.tsx
@ -0,0 +310,4 @@
</div>
<div
style={{
marginTop: "5px",
a258wang commented 2022-08-25 16:25:59 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +322,4 @@
{tooltipData.firstQuartile && (
<div>first quartile: {tooltipData.firstQuartile}</div>
)}
{tooltipData.min && <div>min: {tooltipData.min}</div>}
a258wang commented 2022-08-25 16:32:14 -04:00
Owner
Copy Link

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
pages/playground.tsx Outdated
@ -10,3 +15,3 @@
export default function Home() {
return (
<div className={styles.page}>
<div className={styles.page} suppressHydrationWarning>
a258wang commented 2022-08-25 16:38:45 -04:00
Owner
Copy Link

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 2022-08-25 21:00:09 -04:00
a258wang left a comment
Owner
Copy Link

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
a258wang commented 2022-08-25 22:56:39 -04:00
Owner
Copy Link

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 2022-08-28 16:42:23 -04:00
continuous-integration/drone/push Build is passing Details
dd0360e99d refactor + document code
a258wang reviewed 2022-08-29 21:09:24 -04:00
a258wang left a comment
Owner
Copy Link

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!
components/Boxplot.module.css Outdated
@ -0,0 +10,4 @@
.tooltip {
background-color: var(--label);
color: var(--primary-background);
padding: calc(10rem / 16);
a258wang commented 2022-08-29 21:07:17 -04:00
Owner
Copy Link

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
components/Boxplot.module.css Outdated
@ -0,0 +21,4 @@
.tooltip .toolTipData {
margin-top: 5px;
margin-bottom: 10px;
font-size: 16px;
a258wang commented 2022-08-29 20:48:36 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +274,4 @@
toolTipLeftOffset,
tooltipData: {
min: getMin(d),
max: getMax(d),
a258wang commented 2022-08-29 20:59:44 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +309,4 @@
hideTooltip();
},
}}
boxProps={{
a258wang commented 2022-08-29 21:03:48 -04:00
Owner
Copy Link

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
components/Boxplot.tsx Outdated
@ -0,0 +378,4 @@
{tooltipData.firstQuartile ? (
<p>first quartile: {tooltipData.firstQuartile}</p>
) : null}
{tooltipData.min ? <p>min: {tooltipData.min}</p> : null}
a258wang commented 2022-08-29 21:01:28 -04:00
Owner
Copy Link

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.
a258wang commented 2022-08-29 21:02:46 -04:00
Owner
Copy Link

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 2022-08-31 21:14:56 -04:00
continuous-integration/drone/push Build is passing Details
ecbe05530b refactor code + remove stroke on box
e26chiu added 1 commit 2022-09-01 23:55:35 -04:00
continuous-integration/drone/push Build is passing Details
a703c60824 fix package-lock + refactor variables
a258wang approved these changes 2022-09-02 22:12:30 -04:00
a258wang left a comment
Owner
Copy Link

🚢

🚢
components/Boxplot.module.css
@ -0,0 +15,4 @@
}
.tooltip .category {
margin: 10px 0 0 0;
a258wang commented 2022-09-02 22:03:38 -04:00
Owner
Copy Link

NIT: calc(10rem / 16)

NIT: `calc(10rem / 16)`
e26chiu marked this conversation as resolved
e26chiu added 1 commit 2022-09-03 10:47:08 -04:00
continuous-integration/drone/push Build is passing Details
a188a8a3b4 refactor px to rem
e26chiu added 1 commit 2022-09-03 11:02:26 -04:00
e26chiu merged commit 9526f1b0f5 into main 2022-09-03 11:13:59 -04:00
e26chiu deleted branch boxplot-component 2022-09-03 11:13:59 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
a258wang
Labels
Clear labels   
Bug

   
Component

   
Config

   
Good First Issue

   
Low-Priority

   
Page
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#34
No description provided.
Delete Branch "boxplot-component"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?