mithi / hexapod

Blazing fast hexapod robot simulator for the web.
https://hexapod.netlify.app
Apache License 2.0
579 stars 70 forks source link

Move: hexapod/plotter to templates/plotter #140

Closed 2Shar18 closed 4 years ago

2Shar18 commented 4 years ago

Closes #138 This is my first contribution and PR, hope I am doing it in a proper way. After making the changes, I tried testing and running the project with npm test and npm start with complete success.

codecov[bot] commented 4 years ago

Codecov Report

Merging #140 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #140   +/-   ##
=======================================
  Coverage   75.75%   75.75%           
=======================================
  Files          58       58           
  Lines        1452     1452           
  Branches      178      178           
=======================================
  Hits         1100     1100           
  Misses        306      306           
  Partials       46       46           
Impacted Files Coverage Δ
src/components/HexapodPlot.js 88.88% <ø> (ø)
src/templates/plotter.js 97.67% <ø> (ø)

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 cdd2f1e...00e8541. Read the comment docs.

mithi commented 4 years ago

Closes #138 This is my first contribution and PR, hope I am doing it in a proper way. After making the changes, I tried testing and running the project with npm test and npm start with complete success.

Thanks for your pull request!

The function getNewPlotParams in plotter.js is used by the HexapodPlot component (See ./src/components/HexapodPlot.js) which is currently being imported from ./hexapod/ directory. This is a round about ways of importing.

Can you remove the reference of plotter in ./hexapod/index.js HexapodPlot.js import getNewPlotParams directly ? Thanks

Currently

src/templates/plotter.js ---> src/hexapod/index.js ----> src/component/HexapodPlot.js

Ideally

src/templates/plotter.js -----------------------------------> src/component/HexapodPlot.js

Thanks!

2Shar18 commented 4 years ago

Closes #138 This is my first contribution and PR, hope I am doing it in a proper way. After making the changes, I tried testing and running the project with npm test and npm start with complete success.

Thanks for your pull request!

The function getNewPlotParams in plotter.js is used by the HexapodPlot component (See ./src/components/HexapodPlot.js) which is currently being imported from ./hexapod/ directory. This is a round about ways of importing.

Can you remove the reference of plotter in ./hexapod/index.js HexapodPlot.js import getNewPlotParams directly ? Thanks

Currently

src/templates/plotter.js ---> src/hexapod/index.js ----> src/component/HexapodPlot.js

Ideally

src/templates/plotter.js -----------------------------------> src/component/HexapodPlot.js

Thanks!

Thank you for pointing and very neatly explaining the thing I left out and giving me a chance to re-correct it. I have made the necessary changes and squashed the commits in to a single commit. Kindly have a look at it.

mithi commented 4 years ago

@2Shar18 Thanks! Merged! 😄

2Shar18 commented 4 years ago

@mithi Thank you for giving me the chance. And btw I love the project idea and would try to help and contribute in other issues too. 😀

mithi commented 4 years ago

@mithi Thank you for giving me the chance. And btw I love the project idea and would try to help and contribute in other issues too. 😀

Btw, next time do NOT commit your package.lock because I'm using yarn :D

2Shar18 commented 4 years ago

@mithi Thank you for giving me the chance. And btw I love the project idea and would try to help and contribute in other issues too. 😀

Btw, next time do NOT commit your package.lock because I'm using yarn :D

Oh, sorry for that. Will remember that and try to run with yarn itself.

mithi commented 4 years ago

Oh, sorry for that. Will remember that and try to run with yarn itself.

I'm glad you like this project and thank you so much for your PR 😃