Rewrite in React + Refactor by rafaelklaessen · Pull Request #333 · jenkoian/hacktoberfest-checker

rafaelklaessen · 2018-10-20T01:46:56Z

This is a refactor of the hacktoberfest-checker in React. The backend is now a very light API, separate from the frontend, which is a React app (managed by create-react-app).

The next step would be to figure out what the best way to run the frontend is...Maybe served by the backend, or ran on a different server. Shouldn't be difficult.

Run the frontend with yarn start and the backend with yarn server.

I look forward to your reply! I know this certainly isn't done but would love to eventually see it up and running on your server.

As per screenshot...Everything still looks exactly the same 😄

rafaelklaessen · 2018-10-20T01:49:14Z

Note how the _public folder is the unused one

jenkoian · 2018-10-21T10:15:31Z

Reviewing this now @rafaelklaessen first thoughts... WOW

The next step would be to figure out what the best way to run the frontend is...Maybe served by the backend, or ran on a different server. Shouldn't be difficult.

Outside of simply yarn server && yarn start something like https://www.npmjs.com/package/parallelshell ?

jenkoian · 2018-10-21T10:31:04Z

api/controllers/pr/index.js

@@ -0,0 +1,51 @@
+'use strict';
+
+const _ = require('lodash');


This doesn't appears to be used so can be removed.

rafaelklaessen · 2018-10-21T10:33:47Z

That would be fine for development, but I meant the actual deployment. Originally you had 1 server, now you'll need 2 (static file server for React and a server for the backend).
Don't know if that's a problem for you 😄

jenkoian · 2018-10-21T10:34:47Z

api/controllers/pr/index.js

+  const github = req.app.get('github');
+  const username = req.query.username;
+
+  var hostname = process.env.APP_URL || `${req.protocol}://${req.headers.host}`;


Not used can be removed.

jenkoian · 2018-10-21T10:39:24Z

package.json

+    "not dead",
+    "not ie <= 11",
+    "not op_mini all"
+  ]


I didn't know about browserslist before, nice!

Comes with react-scripts 😄

jenkoian · 2018-10-21T11:07:08Z

src/pages/User/components/PullRequests/ShareButtons.js

+      <a
+        target="_blank"
+        className="fb-xfbml-parse-ignore bg-blue-dark text-white rounded px-2 py-1 pointer text-white no-underline text-sm"
+        href={`https://www.facebook.com/sharer/sharer.php?u=${window.location.origin}/username/${username}`}


Shall we use the hostname in place of window.location.origin?

What is the benefit of that? It's more difficult to use such a variable, and it isn't available to the frontend.

Consistency I guess 🤷‍♂️ it's no biggie. You'll have to excuse my lack of react knowledge here so apologies if this is a bad question, but we're using it here https://github.com/jenkoian/hacktoberfest-checker/pull/333/files#diff-af68e82a3010f5e2e3da304e5fe5caffR45 so could we not pass it through as a prop?

No need for passing it through as a prop, it's available everywhere through process.env.REACT_APP_HOSTNAME. Will change it 😄

jenkoian · 2018-10-21T11:14:12Z

I pushed some spacing fixes which ESLint as well as my IDE were reporting.

Generally, STUNNING WORK. I had started a react refactor as per #319 but got nowhere near as far as you have here.

A few questions (mostly ESLint things) but this is really really close to merge. THANK YOU for submitting 😃

rafaelklaessen · 2018-10-21T11:53:37Z

Some other food for thought, what about 2 spaces rather than 4?

Seems to be the default for the vast majority of React apps, and with 4 spaces everything gets shifted to the right a lot, thereby making code (particularly JSX) more difficult to read. I think...:smile:

jenkoian · 2018-10-21T12:03:17Z

@rafaelklaessen yeah thought you might pushback on that 😄 I tend to prefer 4 spaces as I struggle to see the indentation of just 2 😄 perhaps just me or my IDE though and you're right in JS world it does seem that 2 spaces is the norm. If you're happy to update (including .editorconfig and eslint stuff) then happy to go with 2.

rafaelklaessen · 2018-10-21T15:35:04Z

Moved from ESLint to Prettier because it's just so much more simple, and, opposed to ESLint, it just works(TM) with the latest babel presets.
Old eslint commands still work.

jenkoian · 2018-10-21T15:50:00Z

Thanks! Almost there. Could you update the 'Running the app' section of the README? If you could do the docker stuff that'd be great but no worries if not.

rafaelklaessen · 2018-10-21T18:33:35Z

Done! 😄 Start command is still the same, but it now uses parallelshell.

(Didn't update the docker stuff - I'm not familiar with it.)

jenkoian · 2018-10-22T10:08:44Z

@rafaelklaessen thanks! Gonna give it another look later before merge, but probably the greatest thing I've ever seen 👏

jenkoian · 2018-10-25T09:09:09Z

Hey sorry this is taking so long, need to find time to try this out with deployment but time is something in short supply for me right now. Hopefully soon!

jenkoian · 2018-10-25T20:38:08Z

Sorry got no time at mo to run this myself but doing some reading https://facebook.github.io/create-react-app/docs/deployment#other-solutions seems like what we need? Are you able to try this out locally your end and if it works make the required updates?

rafaelklaessen · 2018-10-25T20:57:24Z

So you do want to combine the React and Express server?
I'm sure it will work...I've run create-react-app on Apache 😛

jenkoian · 2018-10-25T21:10:38Z

That seems the easiest thing for me...

…

On 25 Oct 2018, at 21:57, Rafael Klaessen ***@***.***> wrote: So you do want to combine the React and Express server? I'm sure it will work...I've run create-react-app on Apache 😛 — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

jenkoian · 2018-10-26T13:26:42Z

@rafaelklaessen just to expand on that, it doesn't make sense to me to run the front end through a server as well as the back end, I mean it makes sense for development but not for production. I'm very very new to react though so perhaps it's just me 🤷‍♂️

rafaelklaessen · 2018-10-26T19:26:11Z

Well, it isn't uncommon to have 2 servers. People use 2 servers to maintain separation of concerns, especially for larger apps.

For hacktoberfest-checker, especially since frontend and backend are using the same configs, using one server indeed makes more sense.

jenkoian · 2018-10-26T20:35:37Z

I've pushed a commit which works for me locally. Let me know if you're happy and i'll update the deployment scripts and merge!

rafaelklaessen · 2018-10-27T12:35:02Z

Fixed some tiny issues...There was a space missing in the footer, the build folder wasn't in .gitignore and the static serving didn't use an arrow function.

Running both frontend and backend on one server works great 😄

jenkoian · 2018-10-27T15:16:16Z

Thanks realised it may be worth a note in the .env.example about the port being same on both when running in prod mode too

…

On 27 Oct 2018, at 13:35, Rafael Klaessen ***@***.***> wrote: Fixed some tiny issues...There was a space missing in the footer, the build folder wasn't in .gitignore and the static serving didn't use an arrow function. Running both frontend and backend on one server works great. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

rafaelklaessen · 2018-10-27T15:52:03Z

I updated .env.example 😄

Let me know if you're OK with this phrasing.

jenkoian · 2018-10-29T08:47:51Z

Merged! Thanks so much 👍

rafaelklaessen added 20 commits October 20, 2018 00:11

Added react-scripts

875e858

Move Tailwind

44f2013

Setup base pages

128d624

Added me page

020fd03

Move Footer & GithubCorner to React, update .env

cec6455

More styling, GithubCorner, reminder and footer are now showing

f58edda

Fixed fonts

e133b4e

Update not found page

53f0e82

Reorganised pages, began working on username form

0c70e48

Finished TimeMessage

297cef7

Finished UsernameForm

43abec6

Began simplifying api

064c0c9

Further simplified API

726209d

Further simplified API

e9e06b5

API now works too

e51cbd8

Setup pr list file structure

116caf1

Began working on PR list

5b566f6

Finished PR list

cb04a8a

Fix spaces

d693d00

Remove handlebars templates

e419b10

rafaelklaessen added 4 commits October 20, 2018 15:01

Removed old public folder

6a3cbfe

Spacing fixes, better error handling

a06c702

Fix share buttons

eba82a4

Remove unused dependencies

5330beb

jenkoian reviewed Oct 21, 2018

View reviewed changes

Styling and spacing fixes

7ae0783

rafaelklaessen added 2 commits October 21, 2018 17:28

Replace eslint with prettier, use 2 spaces

bb99061

Use REACT_APP_HOSTNAME

cdb1fa8

Updated README and start commands

90b8f54

Fix env files

349ea9b

Allow react to built and the web app run from a single server

f9b1965

Add build folder to .gitignore, fix missing space & use arrow function

cb5cd89

Updated .env.example

47387f3

Ian Jenkins added 2 commits October 29, 2018 08:36

Update start script ready for deploy

4ef7711

Update Dockerfile and README

22a27ea

jenkoian merged commit 7d66365 into jenkoian:master Oct 29, 2018

Uh oh!

Conversation

rafaelklaessen commented Oct 20, 2018

Uh oh!

rafaelklaessen commented Oct 20, 2018

Uh oh!

jenkoian commented Oct 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelklaessen commented Oct 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenkoian commented Oct 21, 2018

Uh oh!

rafaelklaessen commented Oct 21, 2018

Uh oh!

jenkoian commented Oct 21, 2018

Uh oh!

rafaelklaessen commented Oct 21, 2018

Uh oh!

jenkoian commented Oct 21, 2018

Uh oh!

rafaelklaessen commented Oct 21, 2018

Uh oh!

jenkoian commented Oct 22, 2018

Uh oh!

jenkoian commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenkoian commented Oct 25, 2018

Uh oh!

rafaelklaessen commented Oct 25, 2018

Uh oh!

jenkoian commented Oct 25, 2018 via email

Uh oh!

jenkoian commented Oct 26, 2018

Uh oh!

rafaelklaessen commented Oct 26, 2018

Uh oh!

jenkoian commented Oct 26, 2018

Uh oh!

rafaelklaessen commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenkoian commented Oct 27, 2018 via email

Uh oh!

rafaelklaessen commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jenkoian commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jenkoian commented Oct 25, 2018 •

edited

Loading

rafaelklaessen commented Oct 27, 2018 •

edited

Loading

rafaelklaessen commented Oct 27, 2018 •

edited

Loading