Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because several core requirements and checklist rules are not met, which will likely break tests and expected behavior.
Critical issues to fix:
- Use an empty string for “no selection,” not null:
- Lines with conditions like
this.state.selectedGood !== nullmust check''or use truthiness. - Clear actions must set
selectedGood: ''(e.g., ClearButton and RemoveButton). This is explicitly required and tests will assert it.
- Lines with conditions like
- Button visibility logic is incorrect:
- When
selectedGood === '', show AddButton for all goods. - When a good is selected, hide all AddButtons and show a RemoveButton only for the selected row. Currently AddButtons still appear for non-selected rows.
- When
- Replace inline event handlers with named methods (verb-named) and pass references directly:
- Examples to add:
handleSelectGood(good: string),handleClearSelection(),handleRemoveSelection(). - Remove inline arrows like
onClick={() => ...}and wire buttons to these methods.
- Examples to add:
- Add TypeScript types for state (and props if any):
- For example:
interface State { selectedGood: string }andexport class App extends React.Component<{}, State>.
- For example:
Minor improvement:
- Provide an accessible label for the ClearButton instead of disabling the a11y rule (e.g.,
aria-label="Clear selection").
Once you switch to the empty-string sentinel, correct the button conditions, add typed handler methods, and type the component state, the implementation should align with the spec. Good job on rendering the table, using a single source of truth for selection, and applying the selected row class—fixing the above will complete the task.
✨ 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.tsx
Outdated
| export class App extends React.Component { | ||
| state = { | ||
| selectedGood: 'Jam', | ||
| }; |
There was a problem hiding this comment.
The component and state are untyped. The file declares export class App extends React.Component and state = { selectedGood: 'Jam' } without TypeScript types. The task requires a React + TypeScript implementation (use typed state and props). Add a state interface (for example interface State { selectedGood: string }) and type the component as export class App extends React.Component<{}, State>. This ensures selectedGood is consistently a string and helps satisfy the TypeScript requirement.
src/App.tsx
Outdated
| {this.state.selectedGood !== null ? ( | ||
| <button | ||
| data-cy="RemoveButton" | ||
| data-cy="ClearButton" | ||
| type="button" | ||
| className="button is-info" | ||
| > | ||
| - | ||
| </button> | ||
| </td> | ||
|
|
||
| <td data-cy="GoodTitle" className="is-vcentered"> | ||
| Jam | ||
| </td> | ||
| </tr> | ||
| className="delete ml-3" | ||
| onClick={() => this.setState({ selectedGood: null })} | ||
| /> |
There was a problem hiding this comment.
ClearButton rendering and its onClick use null to clear selection (onClick={() => this.setState({ selectedGood: null })}). The description explicitly states: "ClearButton should clear selection by setting an empty string to selectedGood." Change the clear action to set selectedGood: '' exactly. Also, do not use an inline arrow function here—move this into a named handler method (see other comments about handlers).
src/App.tsx
Outdated
| onClick={() => { | ||
| this.setState({ selectedGood: good }); | ||
| }} | ||
| > | ||
| + | ||
| </button> | ||
| ) : ( | ||
| <button | ||
| data-cy="RemoveButton" | ||
| type="button" | ||
| className="button is-info" | ||
| onClick={() => { | ||
| this.setState({ selectedGood: null }); | ||
| }} |
There was a problem hiding this comment.
Inline arrow functions are used for event handlers (e.g. onClick={() => this.setState({ selectedGood: good })}, onClick={() => this.setState({ selectedGood: null })}). This violates checklist items that require separate named methods and avoiding inline event handlers. Quote checklist item #2: "[CODE KNOWLEDGE] - Create separate methods instead of creating inline event handlers (if we do so - we don't create new arrow function after component re-rendering)" and checklist item #3: "[CODE KNOWLEDGE] - If your method get event as argument or don't get any argument, you don't need to create new arrow function. Just pass your method." Refactor these inline handlers into named methods (for example handleSelectGood(good: string), handleClearSelection(), handleRemoveSelection()), name them starting with a verb, and pass the method reference directly to onClick (no wrapping arrow).
src/App.tsx
Outdated
| onClick={() => { | ||
| this.setState({ selectedGood: good }); | ||
| }} | ||
| > | ||
| + | ||
| </button> | ||
| ) : ( | ||
| <button | ||
| data-cy="RemoveButton" | ||
| type="button" | ||
| className="button is-info" | ||
| onClick={() => { | ||
| this.setState({ selectedGood: null }); | ||
| }} |
There was a problem hiding this comment.
There are no separate named handler methods and method naming convention is not followed. The checklist requires method names to start with a verb: "[CODE STYLE] - Method's name should start with a verb (so you could easily tell what your method actually do)". Create handler methods with verb-prefixed names (e.g. handleSelectGood, handleClearSelection, handleRemoveSelection) instead of using inline functions.
| {this.state.selectedGood !== good ? ( | ||
| <button | ||
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| onClick={() => { | ||
| this.setState({ selectedGood: good }); | ||
| }} | ||
| > | ||
| + | ||
| </button> | ||
| ) : ( |
There was a problem hiding this comment.
Add/Remove button rendering logic is incorrect. Current condition shows an AddButton for every good where this.state.selectedGood !== good, which means when some other good is selected AddButtons still appear. The description requires: "Don't show AddButton when a good is selected." and "Show RemoveButton for the selected good to clear selection." Update rendering rules so that:
- When
selectedGoodis an empty string (no selection), show AddButton for all goods. - When a good is selected, hide all AddButtons and show a RemoveButton only for the selected good.
Adjust conditions accordingly.
src/App.tsx
Outdated
| className="button is-info" | ||
| onClick={() => { | ||
| this.setState({ selectedGood: null }); | ||
| }} |
There was a problem hiding this comment.
RemoveButton's onClick also clears selection by setting selectedGood: null (this.setState({ selectedGood: null })), which violates the task requirement. The description states that clearing must set an empty string: "ClearButton should clear selection by setting an empty string to selectedGood." Ensure the RemoveButton and ClearButton both set selectedGood: '' and call a named handler (e.g. handleRemoveSelection) rather than using an inline arrow.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because there are two critical blockers that violate explicit requirements and checklist rules in src/App.tsx.
- Critical issue 1 (functional): The AddButton visibility condition is incorrect. On lines where you render the button inside the goods map, you use
this.state.selectedGood !== good, which shows AddButtons for all non-selected rows even when another good is selected. The requirement says: “Don’t show AddButton when a good is selected.” Fix by rendering AddButtons only whenthis.state.selectedGood === ''and, when a good is selected, show only the RemoveButton for the selected row and nothing for others. - Critical issue 2 (checklist violation): Inline event handler in JSX. On the AddButton you use
onClick={() => this.handleSelectGood(good)}which creates a new arrow each render and violates the checklist. Replace it by passing a handler reference: attach the good viadata-good={good}and updatehandleSelectGoodto accept the event and readevent.currentTarget.dataset.good, then useonClick={this.handleSelectGood}.
Minor improvement (non-blocker): Consider adding an aria-label to the Clear/Delete button instead of disabling the a11y lint rule.
You’ve nailed most of the core behavior (typed state, initial selection “Jam”, title text, selected-row highlighting, clear/remove behavior). Fixing the two items above should bring the solution fully in line with the spec and checklist. Keep going—almost there!
✨ 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
| } | ||
| > | ||
| <td> | ||
| {this.state.selectedGood !== good ? ( |
There was a problem hiding this comment.
This violates the task description: "Don't show AddButton when a good is selected." Currently the code renders AddButton when this.state.selectedGood !== good, which leaves AddButtons visible for non-selected rows while some other good is selected. Change the condition so AddButtons are shown only when there is no selection (e.g., this.state.selectedGood === ''). See the description for the exact requirement .
src/App.tsx
Outdated
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| onClick={() => this.handleSelectGood(good)} |
There was a problem hiding this comment.
This violates checklist items about avoiding inline event handlers. Exact checklist texts:
- "[CODE KNOWLEDGE] - Create separate methods instead of creating inline event handlers (if we do so - we don't create new arrow function after component re-rendering)"
- "[CODE KNOWLEDGE] - If your method get
eventas argument or don't get any argument, you don't need to create new arrow function. Just pass your method."
You have onClick={() => this.handleSelectGood(good)} which creates a new arrow on every render. Instead, attach the good via a data attribute (e.g. data-good={good}) and make handleSelectGood accept the event and read event.currentTarget.dataset.good, then use onClick={this.handleSelectGood}. See the checklist for guidance .
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational approval: You’ve made strong progress—state is correctly typed and initialized to 'Jam', handlers are cleanly passed as references, keys are stable, selection highlighting works, and Clear/Remove behavior is correct. Nice job removing inline arrow handlers and reading data-good in the event handler.
Critical issue to address (src/App.tsx):
- Lines where you render the action cell in
goods.mapcurrently usethis.state.selectedGood !== goodto show anAddButtonfor all non-selected rows. This violates the requirement: “Don’t showAddButtonwhen a good is selected.” - Why it’s wrong: when any good is selected, no
AddButtonshould be visible anywhere; only the selected row should show aRemoveButton. - How to fix: update the conditional rendering logic so that:
- If
this.state.selectedGood === ''→ renderAddButtonfor every row. - Else if
good === this.state.selectedGood→ renderRemoveButtonfor that row only. - Else → render nothing for the action cell.
- If
Minor improvement:
- Accessibility: instead of disabling
jsx-a11y/control-has-associated-labelfor the Clear button in the title, add anaria-label="Clear selection"to the button.
You’re very close—adjusting the action-cell logic will align with the spec and should satisfy the tests. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| {this.state.selectedGood !== good ? ( | ||
| <button | ||
| data-cy="AddButton" | ||
| type="button" | ||
| className="button" | ||
| data-good={good} | ||
| onClick={this.handleSelectGood} | ||
| > | ||
| + | ||
| </button> | ||
| ) : ( | ||
| <button | ||
| data-cy="RemoveButton" | ||
| type="button" | ||
| className="button is-info" | ||
| onClick={this.handleRemoveSelection} | ||
| > | ||
| - | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
This violates the requirement: "Don't show AddButton when a good is selected" (from the task description) . Current condition this.state.selectedGood !== good renders an AddButton for every non-selected row even when some other good is selected. Update the rendering logic in the goods map so that:
- If
this.state.selectedGood === ''=> show theAddButtonfor every row. - Else if
good === this.state.selectedGood=> show theRemoveButtonfor that row only. - Else => render nothing in the action cell for non-selected rows.
This ensures AddButtons are hidden whenever a selection exists and only the selected row shows the RemoveButton, matching the specification and tests.
No description provided.