-
Notifications
You must be signed in to change notification settings - Fork 1.7k
solution #1758
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
base: master
Are you sure you want to change the base?
solution #1758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
I’m requesting changes because the Increase button does not follow the required behavior. In src/App.jsx, your increase handler checks count % 5 using the pre-update value and calls add100() before addOne(), which violates the requirement that it should add 1 first and only then add 100 if the new value is divisible by 5. This will produce an incorrect sequence and likely fail the tests.
What to fix:
- Update
increaseso the divisibility check uses the value after adding 1. Prefer a single functionalsetCountto compute the next state atomically, e.g., computeafterOne = c + 1and returnafterOne + 100whenafterOne % 5 === 0, otherwiseafterOne.
What you did well:
addOneandadd100are correctly implemented with functional updaters and the buttons are wired up properly.
After implementing the fix, run npm test to confirm the behavior matches the spec. Keep going—you’re very close!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| export const App = () => { | ||
| const [count] = useState(0); | ||
| const [count, setCount] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This violates the requirement from the description: "Increase button calls addOne and then, if count is divisible by 5, it additionally calls add100." The current code relies on the outer count and calls add100() before addOne(), so the divisibility check uses the previous value rather than the value after adding 1. Consider using a single functional setCount that computes afterOne = c + 1 and returns afterOne + 100 when afterOne % 5 === 0, otherwise afterOne.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
I’m requesting changes because there are two blockers that violate the task requirements and will likely fail tests. First, in src/App.jsx lines 16–22, the Increase handler checks count % 5 before adding 1 and calls add100() before addOne(). The requirement states: after clicking Increase, it must add 1 first and then, based on the new value, add 100 if divisible by 5. Fix this using a single functional updater so the update is atomic, e.g., compute afterOne = c + 1 and return afterOne + 100 when afterOne % 5 === 0, otherwise afterOne.
Second, the first button label currently reads Add 1 point, but the requirement expects the exact label Add 1. Please update the button text to match exactly. The addOne and add100 handlers are solid—nice use of functional updaters. After applying these fixes, run npm test to ensure all tests pass. Keep going—you’re close!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/App.jsx
Outdated
|
|
||
| <button type="button" className="App__add-one" onClick={addOne}> | ||
| Add 1 | ||
| Add 1 point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button text is Add 1 point but the task requires the button labeled exactly Add 1. Update the text to match the requirement: "The App must display a title and three buttons labeled: Add 1, Add 100, and Increase."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Here is my approval to help you keep progressing. You’ve implemented a clear React component with proper functional updates for addOne and add100, and the UI/labels look correct. However, there’s one critical issue: in src/App.jsx, your increase handler currently checks count % 5 and calls add100() before addOne() (see the logic around the Increase button handler). This conflicts with the requirement that Increase must first add 1 and then, based on that new value, add 100 if divisible by 5.
Why this matters: the sequence should evaluate divisibility after the +1, and it should be an atomic update to avoid race conditions. Suggested fix: use a single functional update like setCount(c => { const afterOne = c + 1; return afterOne % 5 === 0 ? afterOne + 100 : afterOne; }) so the condition uses the post-increment value. Minor follow-up: after updating, run npm test to confirm all tests pass. Overall, solid structure—just adjust the Increase logic and you’ll be set. Great work, keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| export const App = () => { | ||
| const [count] = useState(0); | ||
| const [count, setCount] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The increase implementation currently checks count % 5 and calls add100() before calling addOne(). This violates the requirement that Increase must first add 1 and then, based on that new value, add 100 if divisible by 5. Implement increase as a single atomic update (e.g. use setCount(c => { const afterOne = c + 1; return afterOne % 5 === 0 ? afterOne + 100 : afterOne; })) so the conditional is evaluated after the +1.
No description provided.