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.

Sign up for GitHub

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

Jump to bottom

Fix reducer store logic and add missing dependencies in package.json #102

Merged
merged 7 commits into from
Oct 5, 2019
Merged

Fix reducer store logic and add missing dependencies in package.json #102

merged 7 commits into from
Oct 5, 2019

Conversation

ypahalajani
Copy link
Contributor

This PR addresses #23

@ypahalajani ypahalajani changed the title WIP: Fix reducer store logic and add missing dependencies in package.json Fix reducer store logic and add missing dependencies in package.json Oct 5, 2019
@ypahalajani ypahalajani mentioned this pull request Oct 5, 2019
Closed
ypahalajani
ypahalajani commented Oct 5, 2019
View reviewed changes
.eslintrc Show resolved Hide resolved
@anikethsaha
Copy link
Owner

Awesome Work Man !
Can you please remove the public/js folder. we wont need to carry that workload.

@anikethsaha
Copy link
Owner

Give me some time to review this ! 😃

@ypahalajani
Copy link
Contributor Author

ypahalajani commented Oct 5, 2019 •
edited
Loading

Can you please remove the public/js folder. we wont need to carry that workload.

Shouldn't we remove the complete public/ folder and add it to the .gitignore ?

@ypahalajani
Copy link
Contributor Author

ypahalajani commented Oct 5, 2019 •
edited
Loading

Awesome Work Man !

🤘

@anikethsaha
Copy link
Owner

Shouldn't we remove the complete public/ folder and add it to the .gitignore ?

Ok either way works fine 👍

@ypahalajani
Copy link
Contributor Author

ypahalajani commented Oct 5, 2019 •
edited
Loading

Ok either way works fine 👍

Also, since the webpack chunks out the (bundle) bin/server.js as well (along with the other files in the public/ folder), do we remove that as well and add to the .gitignore?

anikethsaha
anikethsaha requested changes Oct 5, 2019
View reviewed changes
Copy link
Owner

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some suggestions

src/client/Root.js Outdated Show resolved Hide resolved
src/server/index.js Outdated Show resolved Hide resolved
src/server/index.js Outdated Show resolved Hide resolved
.eslintignore Show resolved Hide resolved
@anikethsaha
Copy link
Owner

Also, since the webpack chunks out the (bundle) bin/server.js as well (along with the other files in the public/ folder), do we remove that as well and add to the .gitignore?

Remove them ! #102 (comment)

@ypahalajani
Copy link
Contributor Author

ypahalajani commented Oct 5, 2019 •
edited
Loading

Also, why did this PR drop the code coverage by 0.3% here? I am not able to figure out correctly 🤔

@anikethsaha
Copy link
Owner

Also, why did this PR drop the code coverage by 0.3% here? I am not able to figure out correctly

Leave it, I guess it may be because of modern js codes...not supportable.

@anikethsaha
Copy link
Owner

Done remove the public/html. Sorry I forgot to mention that....
That is the entry for frontend. You add only public/js to .gitignore

anikethsaha
anikethsaha approved these changes Oct 5, 2019
View reviewed changes
Copy link
Owner

@anikethsaha anikethsaha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@anikethsaha anikethsaha merged commit d3a4a5e into anikethsaha:master Oct 5, 2019
@anikethsaha
Copy link
Owner

@all-contributors please add @ypahalajani for infrastructure, code, doc and bug

@allcontributors allcontributors bot mentioned this pull request Oct 5, 2019
Merged
@allcontributors
Copy link
Contributor

@anikethsaha

I've put up a pull request to add @ypahalajani! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anikethsaha anikethsaha anikethsaha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ypahalajani @anikethsaha