Skip to content

react goods-selector#1775

Open
kamilbykow wants to merge 1 commit intomate-academy:masterfrom
kamilbykow:develop
Open

react goods-selector#1775
kamilbykow wants to merge 1 commit intomate-academy:masterfrom
kamilbykow:develop

Conversation

@kamilbykow
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Great job on implementing the task requirements! 🎉 Your code meets all the specified criteria and functions as expected. While there are a few areas for improvement, such as enhancing readability by refactoring the nested ternary operator and using more descriptive state property names, these are minor and do not impact the core functionality. Keep up the good work, and consider these suggestions for future projects to make your code even more maintainable and clear. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

state = {
selectedGood: 'Jam',
isSelected: true,
classNames: `has-background-success-light`,

Choose a reason for hiding this comment

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

The classNames state property is initialized with a string. Consider using a more descriptive name for this property, such as highlightClass, to clarify its purpose.

data-cy="Good"
key={item}
className={
this.state.selectedGood === item ? this.state.classNames : ''

Choose a reason for hiding this comment

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

The ternary operator is used here to conditionally apply a class name. Ensure that the logic correctly reflects the intended behavior, as it might be confusing to read. Consider adding comments to explain the logic if necessary.

Comment on lines +60 to +89
{this.state.selectedGood !== item ? (
<button
data-cy="AddButton"
type="button"
className="button"
onClick={() => {
this.setState({
isSelected: true,
selectedGood: item,
classNames: `has-background-success-light`,
});
}}
>
+
</button>
) : (
<button
data-cy="RemoveButton"
type="button"
className="button is-info"
onClick={() => {
this.setState({
isSelected: false,
selectedGood: '',
});
}}
>
-
</button>
)}

Choose a reason for hiding this comment

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

The nested ternary operator in the JSX can be difficult to read. Consider refactoring this part of the code to improve readability, perhaps by extracting the button rendering logic into a separate function or component.

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.

2 participants