Add server side rendering #853
Conversation
|
@davidsmejia I checked this a couple more times, it should be ready for review. I'm going to update the readme tomorrow to reflect the nextjs changes and also want to investigate if we can improve the code splitting, right now looks like it's generating too many files. |
There was a problem hiding this comment.
Dang @arielsvn great job! Some small notes but just considerations.
Great Job!
src/state/search/actions.js
Outdated
| page = parseInt(page, 10); | ||
| size = parseInt(size, 10); | ||
| page = page ? parseInt(page, 10) : 1; | ||
| size = size ? parseInt(size, 10) : 10; |
There was a problem hiding this comment.
This looks like it would always be a number if not passed, so will throw an error/NaN out if a non-parsable number is passed.
I think this might be a better option
maybeSize = parseInt(size, 10)
size = maybeSize > 0 ? maybeSize : 10
| @@ -68,7 +68,7 @@ export function FilterCategory({ | |||
| setSavedQuery(query); | |||
| onToggleFilter(queryField, filter); | |||
| }} | |||
| checked={activeValues && activeValues.includes(filter)} | |||
| checked={!!activeValues && activeValues.includes(filter)} | |||
There was a problem hiding this comment.
if you are expecting to guard against falsey values... might be nicer to:
Boolean(activeValues)
There was a problem hiding this comment.
ah !! has the same effect and it's used extensively in the project. I didn't know you could also do Boolean(
| @@ -21,5 +24,5 @@ npm-debug.log* | |||
| yarn-debug.log* | |||
| yarn-error.log* | |||
|
|
|||
| # vim | |||
There was a problem hiding this comment.
haha not sure why that got commented out :)
There was a problem hiding this comment.
oh it's just a header explaining the following lines... so i don't accidentally commit my swap files that are generated by vim
There was a problem hiding this comment.
i did not mean to add the trailing white-space though
|
Also we should figure out previews/deploy before merging. Or at least have a roadmap in place |
|
Talked with @davidsmejia and we came up with a plan to deploy this:
|
Issue Number
#844
Purpose/Implementation Notes
This turned out to be more complicated than I was expecting, however, I'm happy with how it's coming along. I deployed a demo to https://refinebio.now.sh/
A few things are missing from this PR in order to be ready:
Find a solution to navigate between pages that don't rely on the URL
statebecause NextJS doesn't support this by default.Fix filters on search
Fix unit tests
Fix integration tests
Compendia page toggle
Remove all packages related to CRA
Add deploy configuration, we talked about Lambda@Edge. Alternatively, now.sh also integrates with Github and can deploy the app automatically.
@davidsmejia feel free to take a look, I'd really appreciate any comments. With the number of changes this includes we'll have to review it carefully.
Types of changes
Functional tests
Demo on https://refinebio.now.sh/
Checklist