simplified code by using the includes method to check if an option is… #677
Conversation
… in the value array
|
Good one |
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| className="dropdown-heading-dropdown-arrow gray" | ||
| className="rmsc__arrow-svg dropdown-heading-dropdown-arrow gray" |
There was a problem hiding this comment.
Instead of using inline styles for the fill and stroke attributes, consider using CSS classes to style the SVG. This would allow you to more easily customize the appearance of the arrow and make the component more reusable.
| }); | ||
|
|
||
| const handleHover = (iexpanded: boolean) => { | ||
| isInternalExpand && shouldToggleOnHover && setExpanded(iexpanded); | ||
| const handleHover = (isExpanded: boolean) => { |
| isInternalExpand && setExpanded(false); | ||
| }, | ||
| [onChange, isInternalExpand] | ||
| ); | ||
|
|
There was a problem hiding this comment.
This will avoid creating a new function on every render, which can improve performance
| </span> | ||
| const getSelectedText = useMemo( | ||
| () => () => value.map((s) => s.label).join(", "), | ||
| [value] |
There was a problem hiding this comment.
using the useMemo hook to memoize the getSelectedText function, to avoid recomputing it on every render.
There was a problem hiding this comment.
Why are you overcomplicating this by returning a function from useMemo ?
Could be simply a string returned from useMemo with no invocation
| return <span>{selectedText || t("allItemsAreSelected")}</span>; | ||
| default: | ||
| return <span>{selectedText || getSelectedText()}</span>; | ||
| } |
There was a problem hiding this comment.
Instead of using a ternary operator to determine the text to display, consider using a switch statement. This would make the code easier to read and understand, and would allow you to add additional cases more easily.
| type="checkbox" | ||
| onChange={onClick} | ||
| checked={checked} | ||
| tabIndex={-1} | ||
| disabled={disabled} | ||
| /> | ||
| <span>{option.label}</span> | ||
| </div> | ||
| </label> | ||
| ); |
There was a problem hiding this comment.
use a more semantic HTML element for the label, such as a label element. This would improve the accessibility of the component and make it easier to style
| @@ -37,7 +37,7 @@ const SelectList = ({ options, onClick, skipIndex }: ISelectListProps) => { | |||
| tabIndex={tabIndex} | |||
| option={o} | |||
| onSelectionChanged={(c) => handleSelectionChanged(o, c)} | |||
| checked={!!value.find((s) => s.value === o.value)} | |||
| checked={value.includes(o)} | |||
| onClick={(e) => onClick(e, tabIndex)} | |||
There was a problem hiding this comment.
simplify code by using the include method.
| stroke: currentColor; | ||
| stroke-width: 2; | ||
| } | ||
|
|
There was a problem hiding this comment.
style SVG element using CSS value instead of writing hardcoded code. now, the user could override this value if he wants.
|
Please let me know if it's need any changes |
The !! operator is a way to convert a value to a boolean. In this case, it is being used to convert the result of the find method to a boolean.
The find method returns the first element in an array that satisfies the provided predicate function. If no element is found, it returns undefined. When !! is used to convert undefined to a boolean, it becomes false.
So, the checked prop is being set to true if find returns an element that satisfies the predicate, and false if no element is found.
One way to rewrite this code would be to use the Boolean function to explicitly convert the result of find to a boolean: