Skip to content

Conversation

@alychoi
Copy link
Contributor

@alychoi alychoi commented Feb 19, 2021

No description provided.

@NicholasLYang
Copy link
Contributor

Hey original author here. I'm a little curious about your decision to include node-sass and styled-components, as the code already uses react-jss for styles. I'd strongly recommend not using another styling library as the bundle size is already pretty inflated. Ditto with semantic-ui.

@alychoi
Copy link
Contributor Author

alychoi commented Mar 2, 2021 via email

@NicholasLYang
Copy link
Contributor

I believe @DJankauskas has some experience making crosswords. Perhaps he can advise. I'd still avoid pulling in styled-components and using react-jss just for general cohesiveness of the code

@DJankauskas
Copy link
Member

Frankly I also pulled in a library when I built this crossword site, although I used a Svelte library that seems like a version of the one the NYT uses. I don't disagree with deciding to use an external library, but I do think you should consider the implications of shipping more code with stuyspec.com. For that reason, I recommend you lazy-load either react-crossword, or ideally, most of the crossword page using code splitting. It's pretty easy to do, and would mean that users only need to download the code necessary when they check out a crossword. I agree with @NicholasLYang that you should aim to use react-jss instead of styled-components.

Two more recommendations I'd add: first, try converting your code here to use Typescript. It really helps making programming in React far less buggy (like making sure you handle values that might be null or undefined.) Second, consider writing a generic CrosswordPage that takes info like the title and clues as props rather than hardcoding a new component for each puzzle. That'll reduce the code you copy and paste and keep code size down.

@dchen278 dchen278 force-pushed the develop branch 2 times, most recently from 6d93366 to 8474878 Compare April 21, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants