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

Feature/react webapp #210

Merged

Conversation

marcofranssen
Copy link
Contributor

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 9, 2018

Codecov Report

Merging #210 into react-web will increase coverage by 0.15%.
The diff coverage is 51.45%.

Impacted file tree graph

@@              Coverage Diff              @@
##           react-web     #210      +/-   ##
=============================================
+ Coverage       37.3%   37.45%   +0.15%     
=============================================
  Files            115      136      +21     
  Lines          22106    22347     +241     
  Branches           0       37      +37     
=============================================
+ Hits            8246     8370     +124     
- Misses         13129    13233     +104     
- Partials         731      744      +13
Impacted Files Coverage Δ
web/src/components/Navigation/Navigation.js 0% <0%> (ø)
web/src/electron-starter.js 0% <0%> (ø)
web/src/registerServiceWorker.js 0% <0%> (ø)
web/src/index.js 0% <0%> (ø)
web/src/pages/styles.js 100% <100%> (ø)
web/src/pages/Home/Home.js 100% <100%> (ø)
web/src/components/ListItemLink/ListItemLink.js 100% <100%> (ø)
web/src/components/AppBar/AppBar.js 100% <100%> (ø)
web/src/pages/About/About.js 100% <100%> (ø)
web/src/pages/NotFound/NotFound.js 100% <100%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9253424...492bafd. Read the comment docs.

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for this large PR!
I've left comments on the main things that I've noticed that need changes.
Even after that, there are some things I'll need to clarify with Thrasher regarding the front end before we can look to merge it
👍

web/src/pages/Donate/Donate.js Outdated Show resolved Hide resolved
web/src/pages/Home/Home.js Outdated Show resolved Hide resolved
web/package.json Outdated Show resolved Hide resolved
web/src/components/MenuItems/MenuItems.js Outdated Show resolved Hide resolved
web/public/index.html Outdated Show resolved Hide resolved
@marcofranssen marcofranssen force-pushed the feature/react-webapp branch 7 times, most recently from 3e5fa04 to feb1c0e Compare November 18, 2018 10:19
@gloriousCode
Copy link
Collaborator

Thanks for making those changes!

A few more things have com up:
npm run electron is having issues with the pathing:
image

npm run electron-dev does not render:
image

@marcofranssen
Copy link
Contributor Author

See the Readme file in the web folder. For electron dev you should do npm start before.

For the other on npm run build as documented in Readme.

@gloriousCode
Copy link
Collaborator

Alright with the npm start for dev. The first screen was with npm run build -> npm run electron still has the pathing issues

@marcofranssen
Copy link
Contributor Author

Let me check that tonight after work. Will have some time then.

@marcofranssen marcofranssen force-pushed the feature/react-webapp branch 5 times, most recently from 7838d3f to 92b730d Compare November 21, 2018 19:15
@marcofranssen
Copy link
Contributor Author

marcofranssen commented Nov 21, 2018

I have asked for help on that pathing issue in following places.

facebook/create-react-app#1718
https://medium.freecodecamp.org/building-an-electron-application-with-create-react-app-97945861647c

On the other hand we can continue development also on the browser, so do we need to wait for the electron fix?

Copy of the explanation I gave on other github issue

"homepage": "./" does not resolve everything in electron.

static resources like images do not work.

import logo from '../../assets/images/logo.svg';

....
....

<img src={logo} className={classes.logo} alt="logo" />

Neither fetching data using relative paths is working. e.g.

let response = fetch('data/donation-addresses.json');

It will try to access them using an absolute path starting with /.

So instead of trying to access at:
data/donation-addresses.json goes to /data/donation-addresses.json

Same for the image:
assets/images/logo.svg goes to /assets/images/logo.svg.

Any ideas on how to resolve that?

@marcofranssen marcofranssen changed the base branch from master to react-web November 26, 2018 08:54
@gloriousCode gloriousCode merged commit e68dcc2 into thrasher-corp:react-web Nov 26, 2018
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.

3 participants