Add Wordcloud Component #27
Labels
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
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…
Reference in New Issue
No description provided.
Delete Branch "shahanneda/add-wordcloud-2"
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?
Add Wordcloud Component
https://shahanneda-add-wordcloud-2-csc-class-profile-staging-snedadah.k8s.csclub.cloud
Closes #9
@ -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;
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.
Might it make sense to just call the prop
className
anyways?Agreed.
@ -0,0 +143,4 @@
const fixedValueGenerator = () => randomSeed;
return (
<VisxWordcloud
words={data}
I tried using
rem
for these values but it's not possible AFAIK since VisxWordCloud accepts number only as its props.Looks pretty good overall so far! Left some comments mostly about code style 🙂
@ -0,0 +1,19 @@
.word:hover {
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?
@ -0,0 +10,4 @@
left: 0;
position: absolute;
background-color: white;
color: var(--navy);
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 ofwhite
, and we can maybe add avar(--label-text)
that's navy coloured (or just use one of the background colour variables, idk).@ -0,0 +11,4 @@
position: absolute;
background-color: white;
color: var(--navy);
box-shadow: var(--card-background) 0px calc(1rem / 16) calc(2rem / 16);
Check the order of the values for
box-shadow
, I don't think colour comes first?Wierd, some SO posts have it come in front first, I think after is more correct though.
@ -0,0 +46,4 @@
fontWeight = 500,
minFontSize = 20,
maxFontSize = 150,
randomSeed = 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 😛)
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 😿
@ -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. */
NIT:
separate
is the correct spelling@ -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 & {
It looks like we're copying a lot of the keys from
WordCloudProps
here, would it be cleaner if we usedOmit<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
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.
@ -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
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.
Good point.
@ -0,0 +147,4 @@
width={width}
height={height}
fontSize={fontSizeSetter}
font="Inconsolata, monospace"
I'm not sure if we should add quotes around
Inconsolata
🤔we need the quotes here, this is in TSX, that issue is only for CSS.
Ahh, my concern is that this string eventually becomes an inline style:
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"}
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?)
Added it as a prop, our figma design matches the rectangular which is why I set that one.
@ -0,0 +154,4 @@
random={fixedValueGenerator}
>
{(cloudWords) =>
cloudWords.map((w, i) => {
NIT: name the variables
word
andidx
or something slightly more descriptive than a single letter 😛Agree on the word, disagree on the index, it is common enough that
i
makes more sense and is cleaner thanidx
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 😛)
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 ?? ""}`}
I would suggest including the index somewhere in the key as well, in case two words have the same text?
Good point
@ -0,0 +166,4 @@
className={styles.word}
textAnchor="middle"
onMouseMove={
((e: React.MouseEvent<SVGTextElement, MouseEvent>) => {
NIT: name this variable as
event
orevt
?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@ -0,0 +183,4 @@
eventSvgCoords.y
);
}
console.log(e, eventSvgCoords);
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...)Oops!
@ -5,3 +5,3 @@
"engines": {
"node": "^16",
"npm": "^8.0.0"
"npm": ">=7"
This seems sus 🤔 any reason for the change?
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. 😢
@ -110,3 +110,3 @@
--label: var(--dark--label);
}
}
}
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)
@ -5,3 +7,3 @@
export default function Home() {
return (
<>
<div>
This is very unimportant, but any reason why we're changing the React fragment to a div? 😅
Merged with your changes to div now.
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.)