-
-
Notifications
You must be signed in to change notification settings - Fork 52
feat: reactify #23
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
feat: reactify #23
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some initial recommendations(because this not now in MVP mode):
- Squash your commits or make them like so that each commit does on thing that you want to keep, for later. That just makes it super easy to rebase and continue QA.
- Remove all the files that you do not really need from CRA. Service workers?
- Make the code formatting consistent. Add a linter config or re-use the setup from the main codebase.
- Make all the code speak one voice, right now it's a bunch of classes thrown in, some random vanilla code thrown in with ES6?
- Use a bundler to minify all the code that you want to ship.
Essentially this is really good candidate for a spike but we should not ship this to production.
|
one note: the pause button works when toggling device toolbar, but when I tried it from a lower end phone it did not. I was wondering if others are experiencing the same thing. |
|
@raisedadead thank you for the review.
at the moment, only keydown event listeners are in vanila because they performed better than react's onKeyPress on my side. I could onKeyPress work if needed. Also, the visualizer.js is in plain javascript. Because it meant to be separate and replaceable by design with other community data vis solutions, I was not should if I need to change that.
I am currently bundling and minifying via npm run build and serve via netlify cli. |
|
last state deployed: https://5d42d25eee60bdfb275c83ad--coderadio.netlify.com/ |
You will need to add the to the config and version control, right? Using a
Is this from your local dev? |
|
Note that the build step (scripts) that you configure should be served off the static location that netlify understands. This should be in the config. |
Replaceable how? People can add these files to our codebase? And we let them choose from a list? If yes, then I don't like the way the integration is done, currently per the code its a direct import. Extremely Fragile IMHO. Breaking the convention of a react based app and If that's the direction, they should be wrapped in their own packages and included via conventional imports, and have their own tests, etc. For now, simplest way forward is making it in react. |
Can you elaborate more on this? |
fc975b7 to
54d15f9
Compare
|
Currently the deploy preview is not available on this PR because of the missing build config. Netlify simply starts serving from the root on this PR. |
|
@raisedadead thanks for the detailed review. I have taken care of these remaining tasks.
Please let me know if you would like me to further modify this branch. |
No description provided.