Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Winter crossword #305

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Winter crossword #305

wants to merge 10 commits into from

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.

None yet

3 participants