Add Wordcloud Component #27

Merged
snedadah merged 29 commits from shahanneda/add-wordcloud-2 into main 2 months ago
Collaborator
Add Wordcloud Component https://shahanneda-add-wordcloud-2-csc-class-profile-staging-snedadah.k8s.csclub.cloud Closes #9
snedadah added 21 commits 2 months ago
snedadah added 1 commit 2 months ago
e413694317
Rebuilt locally
snedadah added 1 commit 2 months ago
ad12be3a7a
Fixed classname issue
snedadah added 1 commit 2 months ago
1e1e134e08
Added docs
snedadah reviewed 2 months ago
/** 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;
Poster
Collaborator

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.
Owner

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

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

Agreed.

Agreed.
a258wang marked this conversation as resolved
snedadah reviewed 2 months ago
const fixedValueGenerator = () => randomSeed;
return (
<VisxWordcloud
words={data}
Poster
Collaborator

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 2 months ago
4370d82de0
Docs
snedadah requested review from j285he 2 months ago
snedadah requested review from a258wang 2 months ago
snedadah requested review from b72zhou 2 months ago
snedadah requested review from e26chiu 2 months ago
a258wang reviewed 2 months ago
a258wang left a comment
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 🙂
.word:hover {
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
left: 0;
position: absolute;
background-color: white;
color: var(--navy);
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
position: absolute;
background-color: white;
color: var(--navy);
box-shadow: var(--card-background) 0px calc(1rem / 16) calc(2rem / 16);
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?
Poster
Collaborator

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
fontWeight = 500,
minFontSize = 20,
maxFontSize = 150,
randomSeed = 0.5,
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 😛)
Poster
Collaborator

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
}
);
/** The internal wordcloud component that actually lays out the word needs to be seperate from the tooltip to prevent extra rerendering. */
Owner

NIT: separate is the correct spelling

NIT: `separate` is the correct spelling
snedadah marked this conversation as resolved
);
/** The internal wordcloud component that actually lays out the word needs to be seperate from the tooltip to prevent extra rerendering. */
type WordCloudWordsProps = WordCloudProps & {
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
Poster
Collaborator

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
) => 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
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.
Poster
Collaborator

Good point.

Good point.
snedadah marked this conversation as resolved
width={width}
height={height}
fontSize={fontSizeSetter}
font="Inconsolata, monospace"
Owner

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

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

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.
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. 😄
fontSize={fontSizeSetter}
font="Inconsolata, monospace"
padding={wordPadding}
spiral={"rectangular"}
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?)
Poster
Collaborator

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
random={fixedValueGenerator}
>
{(cloudWords) =>
cloudWords.map((w, i) => {
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 😛
Poster
Collaborator

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 😛)
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.
cloudWords.map((w, i) => {
return (
<Text
key={`wordcloud-word-${w.text ?? ""}`}
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?
Poster
Collaborator

Good point

Good point
a258wang marked this conversation as resolved
className={styles.word}
textAnchor="middle"
onMouseMove={
((e: React.MouseEvent<SVGTextElement, MouseEvent>) => {
Owner

NIT: name this variable as event or evt?

NIT: name this variable as `event` or `evt`?
Poster
Collaborator

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
eventSvgCoords.y
);
}
console.log(e, eventSvgCoords);
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...)
Poster
Collaborator

Oops!

Oops!
a258wang marked this conversation as resolved
package.json Outdated
"engines": {
"node": "^16",
"npm": "^8.0.0"
"npm": ">=7"
Owner

This seems sus 🤔 any reason for the change?

This seems sus 🤔 any reason for the change?
Poster
Collaborator

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
--label: var(--dark--label);
}
}
}
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
export default function Home() {
return (
<>
<div>
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? 😅
Poster
Collaborator

Merged with your changes to div now.

Merged with your changes to div now.
a258wang marked this conversation as resolved
snedadah added 2 commits 2 months ago
snedadah added 1 commit 2 months ago
ea5f911d8e
Spacing
a258wang approved these changes 2 months ago
a258wang left a comment
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 2 months ago
fd9c665bcc
Rebuilt package-lock.json
snedadah merged commit b586d52f72 into main 2 months ago
snedadah deleted branch shahanneda/add-wordcloud-2 2 months ago
snedadah referenced this issue from a commit 2 months ago

Reviewers

j285he was requested for review 2 months ago
b72zhou was requested for review 2 months ago
e26chiu was requested for review 2 months ago
a258wang approved these changes 2 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as b586d52f72.
Sign in to join this conversation.
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#27
Loading…
There is no content yet.