Add Wordcloud Component #27

Merged
snedadah merged 29 commits from shahanneda/add-wordcloud-2 into main 2022-08-04 02:17:21 -04:00
repo.issues.owner
Add Wordcloud Component https://shahanneda-add-wordcloud-2-csc-class-profile-staging-snedadah.k8s.csclub.cloud Closes #9
snedadah added 21 commits 2022-07-21 23:44:54 -04:00
d638f42e8f
Fix Inconsolata font (#24)
Fixes #23

Kudos to @j285he for noticing that we had commas in `pages/font.css` when we should've had semicolons.

Co-authored-by: Amy Wang <a258wang@csclub.uwaterloo.ca>
Reviewed-on: #24
Reviewed-by: Shahan Neda <snedadah@csclub.uwaterloo.ca>
continuous-integration/drone/push Build is failing Details
9149117115
Removed unused import
continuous-integration/drone/push Build is failing Details
f6443560e7
Updated package.json engine
continuous-integration/drone/push Build is failing Details
8d8eb542d0
Changed capitilization
continuous-integration/drone/push Build is failing Details
a03e85e643
Fixed import
continuous-integration/drone/push Build is failing Details
2e6ab1c039
Readded wordcloud
snedadah added 1 commit 2022-07-21 23:47:24 -04:00
continuous-integration/drone/push Build is passing Details
e413694317
Rebuilt locally
snedadah added 1 commit 2022-07-22 00:01:53 -04:00
continuous-integration/drone/push Build is passing Details
ad12be3a7a
Fixed classname issue
snedadah added 1 commit 2022-07-22 00:52:26 -04:00
continuous-integration/drone/push Build is passing Details
1e1e134e08
Added docs
snedadah reviewed 2022-07-22 00:53:22 -04:00
@ -0,0 +26,4 @@
/** A random seed used for placing the words, change this value to get an alternate placement of words */
randomSeed?: number;
/** ClassName of the wrapper of the wordcloud */
wrapperClassName?: string;
repo.issues.poster
repo.issues.owner

Because the way the visx wordcloud works, I don't think can't set the classname of the actual svg, so the wrapper is the best we can do.

Because the way the visx wordcloud works, I don't think can't set the classname of the actual svg, so the wrapper is the best we can do.
repo.issues.owner

Might it make sense to just call the prop className anyways?

Might it make sense to just call the prop `className` anyways?
repo.issues.poster
repo.issues.owner

Agreed.

Agreed.
a258wang marked this conversation as resolved
snedadah reviewed 2022-07-22 00:54:35 -04:00
@ -0,0 +143,4 @@
const fixedValueGenerator = () => randomSeed;
return (
<VisxWordcloud
words={data}
repo.issues.poster
repo.issues.owner

I tried using rem for these values but it's not possible AFAIK since VisxWordCloud accepts number only as its props.

I tried using `rem` for these values but it's not possible AFAIK since VisxWordCloud accepts number only as its props.
snedadah marked this conversation as resolved
snedadah added 1 commit 2022-07-22 00:55:55 -04:00
continuous-integration/drone/push Build is passing Details
4370d82de0
Docs
snedadah requested review from j285he 2022-07-22 00:57:52 -04:00
snedadah requested review from a258wang 2022-07-22 00:57:52 -04:00
snedadah requested review from b72zhou 2022-07-22 00:57:52 -04:00
snedadah requested review from e26chiu 2022-07-22 00:57:52 -04:00
a258wang reviewed 2022-08-01 00:22:53 -04:00
a258wang left a comment
repo.issues.owner

Looks pretty good overall so far! Left some comments mostly about code style 🙂

Looks pretty good overall so far! Left some comments mostly about code style 🙂
@ -0,0 +1,19 @@
.word:hover {
repo.issues.owner

I noticed when hovering over words that the cursor becomes the text cursor, I think in this context it might be nice to keep it as the default cursor?

I noticed when hovering over words that the cursor becomes the text cursor, I think in this context it might be nice to keep it as the default cursor?
snedadah marked this conversation as resolved
@ -0,0 +10,4 @@
left: 0;
position: absolute;
background-color: white;
color: var(--navy);
repo.issues.owner

Let's try to use theme colours for these instead of white and --navy?

I think it'd make sense to use var(--label) instead of white, and we can maybe add a var(--label-text) that's navy coloured (or just use one of the background colour variables, idk).

Let's try to use theme colours for these instead of `white` and `--navy`? I think it'd make sense to use `var(--label)` instead of `white`, and we can maybe add a `var(--label-text)` that's navy coloured (or just use one of the background colour variables, idk).
snedadah marked this conversation as resolved
@ -0,0 +11,4 @@
position: absolute;
background-color: white;
color: var(--navy);
box-shadow: var(--card-background) 0px calc(1rem / 16) calc(2rem / 16);
repo.issues.owner

Check the order of the values for box-shadow, I don't think colour comes first?

Check the order of the values for `box-shadow`, I don't think colour comes first?
repo.issues.poster
repo.issues.owner

Wierd, some SO posts have it come in front first, I think after is more correct though.

Wierd, some SO posts have it come in front first, I think after is more correct though.
snedadah marked this conversation as resolved
@ -0,0 +46,4 @@
fontWeight = 500,
minFontSize = 20,
maxFontSize = 150,
randomSeed = 0.5,
repo.issues.owner

Does the seed have to fall in the interval [0, 1]? Restrictions like that should probably be mentioned in the docstring, if they exist.

(Also, why 0.5 out of all numbers? I personally would've gone with 42 or 10 or 3.14 or 3 or even like a basic 1 or 0 or something, not 0.5 😛)

Does the seed have to fall in the interval \[0, 1\]? Restrictions like that should probably be mentioned in the docstring, if they exist. (Also, why 0.5 out of all numbers? I personally would've gone with 42 or 10 or 3.14 or 3 or even like a basic 1 or 0 or something, not 0.5 😛)
repo.issues.poster
repo.issues.owner

It's just the value that the visx documentation uses. Good point on the range, I dug into visx docs to find the range and added the comment. What's wrong with 0.5? I'm sure 0.5 doesn't appreciate getting bullied like this 😿

It's just the value that the visx documentation uses. Good point on the range, I dug into visx docs to find the range and added the comment. What's wrong with 0.5? I'm sure 0.5 doesn't appreciate getting bullied like this 😿
a258wang marked this conversation as resolved
@ -0,0 +99,4 @@
}
);
/** The internal wordcloud component that actually lays out the word needs to be seperate from the tooltip to prevent extra rerendering. */
repo.issues.owner

NIT: separate is the correct spelling

NIT: `separate` is the correct spelling
snedadah marked this conversation as resolved
@ -0,0 +100,4 @@
);
/** The internal wordcloud component that actually lays out the word needs to be seperate from the tooltip to prevent extra rerendering. */
type WordCloudWordsProps = WordCloudProps & {
repo.issues.owner

It looks like we're copying a lot of the keys from WordCloudProps here, would it be cleaner if we used Omit<WordCloudProps, ...> & { ... } instead of copying so much?

Omit: https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys
Intersection types (&): https://www.typescriptlang.org/docs/handbook/2/objects.html#intersection-types

It looks like we're copying a lot of the keys from `WordCloudProps` here, would it be cleaner if we used `Omit<WordCloudProps, ...> & { ... }` instead of copying so much? Omit: https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys Intersection types (&): https://www.typescriptlang.org/docs/handbook/2/objects.html#intersection-types
repo.issues.poster
repo.issues.owner

Good point on using Omit. I originally did not want to use the same thing since that requires me to move the default parameters inside this component. However, I think it is better this way.

Good point on using Omit. I originally did not want to use the same thing since that requires me to move the default parameters inside this component. However, I think it is better this way.
a258wang marked this conversation as resolved
@ -0,0 +116,4 @@
) => void;
hideTooltip: () => void;
// These next props are just used to stop the component from updating when it doesnt need to,
// but they are not needed to render the component
repo.issues.owner

I'm a little confused what this comment means, but if I'm understanding it correctly then would it be more succinct to say "These props are used for memoization"?

Also I would recommend explicitly listing which props instead of saying "These next props" -- this makes the comment a bit more future-proof in case someone decides to change the order of the props or add new props or something in the future.

I'm a little confused what this comment means, but if I'm understanding it correctly then would it be more succinct to say "These props are used for memoization"? Also I would recommend explicitly listing which props instead of saying "These next props" -- this makes the comment a bit more future-proof in case someone decides to change the order of the props or add new props or something in the future.
repo.issues.poster
repo.issues.owner

Good point.

Good point.
snedadah marked this conversation as resolved
@ -0,0 +147,4 @@
width={width}
height={height}
fontSize={fontSizeSetter}
font="Inconsolata, monospace"
repo.issues.owner

I'm not sure if we should add quotes around Inconsolata 🤔

I'm not sure if we should add quotes around `Inconsolata` 🤔
repo.issues.poster
repo.issues.owner

we need the quotes here, this is in TSX, that issue is only for CSS.

we need the quotes here, this is in TSX, that issue is only for CSS.
repo.issues.owner

Ahh, my concern is that this string eventually becomes an inline style:

<text transform="translate(8, 0)" fill="var(--primary-accent)" font-size="150" font-family="Inconsolata, monospace" font-weight="500" class="WordCloud_word__4_u0u" text-anchor="middle"><tspan x="0" dy="0em">Python</tspan></text>

Looking at staging the font seems to be okay, but I'm not sure if that's because it's inline or if it's because Inconsolata doesn't have any spaces or if it's because of something else. But also I just tried adding quotation marks to the HTMl and it didn't seem to do anything, so yeah we're probably fine. 😄

Ahh, my concern is that this string eventually becomes an inline style: ```html <text transform="translate(8, 0)" fill="var(--primary-accent)" font-size="150" font-family="Inconsolata, monospace" font-weight="500" class="WordCloud_word__4_u0u" text-anchor="middle"><tspan x="0" dy="0em">Python</tspan></text> ``` Looking at staging the font seems to be okay, but I'm not sure if that's because it's inline or if it's because `Inconsolata` doesn't have any spaces or if it's because of something else. But also I just tried adding quotation marks to the HTMl and it didn't seem to do anything, so yeah we're probably fine. 😄
@ -0,0 +149,4 @@
fontSize={fontSizeSetter}
font="Inconsolata, monospace"
padding={wordPadding}
spiral={"rectangular"}
repo.issues.owner

Any particular reason why a rectagular spiral, and not an archimedean spiral? (Maybe it'd be nice to be able to control this as a prop?)

Any particular reason why a rectagular spiral, and not an archimedean spiral? (Maybe it'd be nice to be able to control this as a prop?)
repo.issues.poster
repo.issues.owner

Added it as a prop, our figma design matches the rectangular which is why I set that one.

Added it as a prop, our figma design matches the rectangular which is why I set that one.
a258wang marked this conversation as resolved
@ -0,0 +154,4 @@
random={fixedValueGenerator}
>
{(cloudWords) =>
cloudWords.map((w, i) => {
repo.issues.owner

NIT: name the variables word and idx or something slightly more descriptive than a single letter 😛

NIT: name the variables `word` and `idx` or something slightly more descriptive than a single letter 😛
repo.issues.poster
repo.issues.owner

Agree on the word, disagree on the index, it is common enough that i makes more sense and is cleaner than idx IMO. Also, this code is exactly from the visx docs.
However, I renamed to ensure consistency with the bar graph code. (which uses a confusing one letter name in this same place for the first argument 😛)

Agree on the word, disagree on the index, it is common enough that `i` makes more sense and is cleaner than `idx` IMO. Also, this code is exactly from the visx docs. However, I renamed to ensure consistency with the bar graph code. (which uses a confusing one letter name in this same place for the first argument 😛)
repo.issues.owner

Fair enough. Also, if someone used a confusing one-letter variable in a different component, feel free to rectify that and then also yell at that person the next time you talk to them.

Fair enough. Also, if someone used a confusing one-letter variable in a different component, feel free to rectify that and then also yell at that person the next time you talk to them.
@ -0,0 +157,4 @@
cloudWords.map((w, i) => {
return (
<Text
key={`wordcloud-word-${w.text ?? ""}`}
repo.issues.owner

I would suggest including the index somewhere in the key as well, in case two words have the same text?

I would suggest including the index somewhere in the key as well, in case two words have the same text?
repo.issues.poster
repo.issues.owner

Good point

Good point
a258wang marked this conversation as resolved
@ -0,0 +166,4 @@
className={styles.word}
textAnchor="middle"
onMouseMove={
((e: React.MouseEvent<SVGTextElement, MouseEvent>) => {
repo.issues.owner

NIT: name this variable as event or evt?

NIT: name this variable as `event` or `evt`?
repo.issues.poster
repo.issues.owner

I think in the instance of events, it is very common practice to use e, and IMO confusing to use evt (extreme value thereom!? 😆 ). In fact in the react docs (and almost all other docs) they use e. https://reactjs.org/docs/handling-events.html

I think in the instance of events, it is very common practice to use e, and IMO confusing to use `evt` (extreme value thereom!? 😆 ). In fact in the react docs (and almost all other docs) they use e. https://reactjs.org/docs/handling-events.html
a258wang marked this conversation as resolved
@ -0,0 +183,4 @@
eventSvgCoords.y
);
}
console.log(e, eventSvgCoords);
repo.issues.owner

Should probably get rid of this 😛

(Maybe we should add some kind of lint rule to check for console.log() etc. in .tsx files or something...)

Should probably get rid of this 😛 (Maybe we should add some kind of lint rule to check for `console.log()` etc. in `.tsx` files or something...)
repo.issues.poster
repo.issues.owner

Oops!

Oops!
a258wang marked this conversation as resolved
package.json Outdated
@ -5,3 +5,3 @@
"engines": {
"node": "^16",
"npm": "^8.0.0"
"npm": ">=7"
repo.issues.owner

This seems sus 🤔 any reason for the change?

This seems sus 🤔 any reason for the change?
repo.issues.poster
repo.issues.owner

sus indeed. I think I was having some trouble with some of the packages and this was one possible fix, I was able to fix it another way though I think, and forgot to revert this. 😢

sus indeed. I think I was having some trouble with some of the packages and this was one possible fix, I was able to fix it another way though I think, and forgot to revert this. 😢
a258wang marked this conversation as resolved
@ -110,3 +110,3 @@
--label: var(--dark--label);
}
}
}
repo.issues.owner

Not sure if a newline has been added or removed here 😂 we should probably edit the vscode settings so everyone's editors behave consistently though

(My personal preference is to have a newline at the end of files, but I don't care that much as long as we're consistent and don't get random changes like this lol)

Not sure if a newline has been added or removed here 😂 we should probably edit the vscode settings so everyone's editors behave consistently though (My personal preference is to have a newline at the end of files, but I don't care *that* much as long as we're consistent and don't get random changes like this lol)
a258wang marked this conversation as resolved
@ -5,3 +7,3 @@
export default function Home() {
return (
<>
<div>
repo.issues.owner

This is very unimportant, but any reason why we're changing the React fragment to a div? 😅

This is very unimportant, but any reason why we're changing the React fragment to a div? 😅
repo.issues.poster
repo.issues.owner

Merged with your changes to div now.

Merged with your changes to div now.
a258wang marked this conversation as resolved
snedadah added 2 commits 2022-08-02 23:24:31 -04:00
snedadah added 1 commit 2022-08-02 23:28:03 -04:00
continuous-integration/drone/push Build is passing Details
ea5f911d8e
Spacing
a258wang approved these changes 2022-08-04 00:58:32 -04:00
a258wang left a comment
repo.issues.owner

This looks awesome, thanks for working on it and iterating on the feedback! Last thing is to please run npm install with the package.json that you have here in order to make sure that we are committing the "correct" package-lock.json. Other than this, I think this is good to merge! (We can always come back and tweak things later if necessary.)

This looks awesome, thanks for working on it and iterating on the feedback! Last thing is to please run `npm install` with the package.json that you have here in order to make sure that we are committing the "correct" package-lock.json. Other than this, I think this is good to merge! (We can always come back and tweak things later if necessary.)
snedadah added 1 commit 2022-08-04 02:11:04 -04:00
continuous-integration/drone/push Build is passing Details
fd9c665bcc
Rebuilt package-lock.json
snedadah merged commit b586d52f72 into main 2022-08-04 02:17:21 -04:00
snedadah deleted branch shahanneda/add-wordcloud-2 2022-08-04 02:17:21 -04:00
Sign in to join this conversation.
No Milestone
No project
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#27
No description provided.