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

Milestone 5 Complete #5

Merged
merged 3 commits into from
Oct 9, 2020
Merged

Milestone 5 Complete #5

merged 3 commits into from
Oct 9, 2020

Conversation

ivanid22
Copy link
Owner

@ivanid22 ivanid22 commented Oct 8, 2020

I've just completed milestone 5 of the React calculator project. In this assignment, I've connected the calculator logic to the React App component. I've had to refactor the calculate method a bit to be able to keep track of the application state independently.
Looking forward to your feedback!

Thanks!

Copy link

@Forison Forison left a comment

Choose a reason for hiding this comment

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

Status:: Changes required❌.

🙋‍♂️ @ivanid22

You have done a good job on this one, I like the way the display could accommodate numbers without the number overflowing. Your calculator is almost as required. Let us address these few issues to get the project approved🤗.

  • Kindly add an image of the application in the readme.
  • The readme should include how to set up and use the project on ones's local machine.
  • When a number is divided by zero the application breaks, it ought to return undefined instead.
  • When % is selected on a single number ie 10 the application should return 0.1
  • When % is selected on an operation ie 10 +20 the application should return 0.3
  • this way we can perform further computation and have a more robust calculator.
  • Check comment below in relation to moving code into calculate.js
    [OPTIONAL]
  • Kindly add a description to all repository you create on Github.
  • The application can have a better user experience if the selected operation and the next value are all visible as the user performs a computation.

That is kindly address the issue and submit a re-review👨‍🔧.

Happy coding👨‍💻

Addo Forison

src/components/ButtonPanel.js Outdated Show resolved Hide resolved
src/components/App.js Outdated Show resolved Hide resolved
Copy link

@whiz25 whiz25 left a comment

Choose a reason for hiding this comment

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

Approved 👏

You have done a good job fixing the issues addressed by the previous reviewer. Merge and proceed to the next project.

Martin Afani

@ivanid22 ivanid22 merged commit 0f77fd3 into development Oct 9, 2020
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