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

Add Wordcloud Component #27

Merged
snedadah merged 29 commits from shahanneda/add-wordcloud-2 into main 2022-08-04 02:17:21 -04:00
Conversation 0 Commits 29 Files Changed 9 +1 -1
1 changed files with 1 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4370d82de0 - Show all commits

2
components/WordCloud.tsx
View File

@ -99,6 +99,7 @@ export const WordCloud = withTooltip(
}
);
/** The internal wordcloud component that actually lays out the word needs to be seperate from the tooltip to prevent extra rerendering. */
type WordCloudWordsProps = WordCloudProps & {
a258wang marked this conversation as resolved
a258wang commented 2022-07-31 23:59:21 -04:00
Review
Copy Link

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
👍 1
snedadah commented 2022-08-02 23:26:27 -04:00
Review
Copy Link

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.
👍 1
data: Array<WordData>;
width: number;
@ -118,7 +119,6 @@ type WordCloudWordsProps = WordCloudProps & {
// but they are not needed to render the component
snedadah marked this conversation as resolved
a258wang commented 2022-07-31 23:50:47 -04:00
Review
Copy Link

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.
snedadah commented 2022-08-02 23:20:18 -04:00
Review
Copy Link

Good point.

Good point.
tooltipLeft?: number;
tooltipTop?: number;
// className?: string;
};
const WordCloudWords: React.FC<WordCloudWordsProps> = ({
width,