Add Grouped Bar Graph #65

a258wang · 2022-09-21T15:23:42-04:00

a258wang commented

2022-09-21 15:23:42 -04:00

Closes #2

Notes:

Named the component the GroupedBarGraph since it can support bar groups of > 2 bars.
The horizontal graph is mostly a copy of the vertical graph, with slight modifications. This ended up being the ~~laziest~~ easiest way to implement it.

Staging doesn't seem to be reflecting the latest commit. 🤔

Closes #2 Notes: - Named the component the GroupedBarGraph since it can support bar groups of > 2 bars. - The horizontal graph is mostly a copy of the vertical graph, with slight modifications. This ended up being the ~~laziest~~ easiest way to implement it. Staging doesn't seem to be reflecting the latest commit. 🤔

a258wang added 6 commits 2022-09-21 15:23:43 -04:00

ec912f9a9f Add basic grouped bar graph

582e1c84ab Add background and axes

8090209b30 Add hover effects

194bfa38a0 Add legend

0a8bb8b4ad Update package-lock.json

continuous-integration/drone/push Build is passing Details

059c63a843 Add horizontal grouped bar graph

snedadah added 1 commit 2022-09-21 20:16:03 -04:00

continuous-integration/drone/push Build is passing Details

a0be0f993f

Empty commit to test staging

snedadah commented

2022-09-21 20:24:31 -04:00

Closes #2

Notes:

Named the component the GroupedBarGraph since it can support bar groups of > 2 bars.

The horizontal graph is mostly a copy of the vertical graph, with slight modifications. This ended up being the ~~laziest~~ easiest way to implement it.

Staging doesn't seem to be reflecting the latest commit. 🤔

Hmmm... staging issue is very strange .... 🤔

> Closes #2 > > Notes: > - Named the component the GroupedBarGraph since it can support bar groups of > 2 bars. > - The horizontal graph is mostly a copy of the vertical graph, with slight modifications. This ended up being the ~~laziest~~ easiest way to implement it. > > Staging doesn't seem to be reflecting the latest commit. 🤔 Hmmm... staging issue is very strange .... 🤔

snedadah requested review from snedadah 2022-09-28 22:11:53 -04:00

snedadah approved these changes 2022-10-01 20:37:22 -04:00

snedadah left a comment

Nice work!!

Its unfortunate that a lot of code is repeated for the horizontal and vertical, but there is not much we can do about this, lets merge this so we can get closer to finishing! We can always refactor for later iterations of the class profile. I flagged some confusing parts for me when reading, if we comment the confusing parts a bit more to clear things up a bit it would help for future people who might want to refactor these components.

(Also, what was the reason that we need both a horizontal and a vertical one again? I would think just one would have been enough.)

Nice work!! Its unfortunate that a lot of code is repeated for the horizontal and vertical, but there is not much we can do about this, lets merge this so we can get closer to finishing! We can always refactor for later iterations of the class profile. I flagged some confusing parts for me when reading, if we comment the confusing parts a bit more to clear things up a bit it would help for future people who might want to refactor these components. (Also, what was the reason that we need both a horizontal and a vertical one again? I would think just one would have been enough.)

components/GroupedBarGraph.module.css

													
				@ -0,0 +11,4 @@

				.barText {

				  visibility: hidden;

				  font-family: "Inconsolata", monospace;

snedadah commented

2022-10-01 20:11:02 -04:00

Do we need to manually set the font here? Do the fonts set in _app.css not apply? It's unforntunate if we do, since incase we ever need to change the font in the future, it will require changing all these too :(

Do we need to manually set the font here? Do the fonts set in `_app.css` not apply? It's unforntunate if we do, since incase we ever need to change the font in the future, it will require changing all these too :(

a258wang commented

2022-10-02 12:36:56 -04:00

Yes, unfortunately it looks like we need to specify the font or else visx will default to Arial or some other font that we don't want. I'll create a ticket to factor out the font-family into a CSS variable that we can use throughout the site for cases like this.

components/GroupedBarGraph.tsx Outdated

													
				@ -0,0 +427,4 @@

				            {data.map((d, idx) => {

				              const barName = `${getCategory(d)}-${idx}`;

				              const barWidth = categoryScale.bandwidth();

				              const backgroundBarWidth = barWidth / (1 - barPadding);

snedadah commented

2022-09-28 22:53:01 -04:00

What if barPadding=1 or barPadding > 1? I think we should put a comment above where we defined barPadding:

// barPadding must in the range [0,1)
const barPadding = 0.2;

Same comment applies to the component above as well.

What if `barPadding=1` or barPadding > 1? I think we should put a comment above where we defined barPadding: ``` // barPadding must in the range [0,1) const barPadding = 0.2; ``` Same comment applies to the component above as well.

👍 1

a258wang marked this conversation as resolved

components/GroupedBarGraph.tsx

													
				@ -0,0 +435,4 @@

				                  x={0}

				                  y={

				                    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion

				                    categoryScale(getCategory(d))! -

snedadah commented

2022-09-28 22:40:42 -04:00

is it possible for this to actually fail due to bad user data or is the ts compiler just being dumb?

a258wang commented

2022-10-02 12:46:55 -04:00

The way the code is written, this shouldn't ever be null. The TS compiler is smart, but not smart enough to detect this.

We should probably consider disabling the ESLint no-non-null-assertion rule since I don't think our team has a tendency to abuse non-null assertions.

The way the code is written, this shouldn't ever be null. The TS compiler is smart, but not smart enough to detect this. We should probably consider disabling the ESLint no-non-null-assertion rule since I don't think our team has a tendency to abuse non-null assertions.

components/GroupedBarGraph.tsx

													
				@ -0,0 +564,4 @@

				  isHorizontal?: boolean;

				}

				function HoverableBar(props: HoverableBarProps) {

snedadah commented

2022-10-01 20:28:22 -04:00

Nice, smart to factor this component out!

a258wang marked this conversation as resolved

components/GroupedBarGraph.tsx Outdated

													
				@ -0,0 +593,4 @@

				        width={bar.width}

				        height={bar.height}

				        fill={isHovered && hoverFillColor ? hoverFillColor : bar.color}

				        filter={isHovered ? `url(#glow-${colorId})` : undefined}

snedadah commented

2022-09-28 22:43:31 -04:00

could we leave a quick comment here explaining what the url(#glow-${colorId}) is?

could we leave a quick comment here explaining what the `url(#glow-${colorId})` is?

👍 1

a258wang marked this conversation as resolved

components/GroupedBarGraph.tsx Outdated

													
				@ -0,0 +597,4 @@

				      />

				      <Text

				        className={styles.barText}

				        x={isHorizontal ? bar.width - 12 : bar.x + bar.width / 2}

snedadah commented

2022-10-01 20:16:59 -04:00

Pretty confusing for me, for example why the magic number - 12 here? I think it would be better if that was extracted into a constant, which would shed some light on to why we need to - 12.

👍 1

a258wang marked this conversation as resolved

a258wang force-pushed a258wang-double-bar-graph from a0be0f993f to ab5161d977

2022-10-02 12:52:15 -04:00

Compare

a258wang commented

2022-10-02 12:52:52 -04:00