-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
simplified code by using the includes method to check if an option is… #677
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,17 @@ | ||
import React from "react"; | ||
interface IArrowProps { | ||
expanded: boolean | undefined; | ||
} | ||
|
||
export const Arrow = ({ expanded }) => ( | ||
export const Arrow = ({ expanded }: IArrowProps) => ( | ||
<svg | ||
width="24" | ||
height="24" | ||
fill="none" | ||
stroke="currentColor" | ||
strokeWidth="2" | ||
className="dropdown-heading-dropdown-arrow gray" | ||
className="rmsc__arrow-svg dropdown-heading-dropdown-arrow gray" | ||
> | ||
<path d={expanded ? "M18 15 12 9 6 15" : "M6 9L12 15 18 9"} /> | ||
<path | ||
className="rmsc__arrow-path" | ||
d={expanded ? "M18 15 12 9 6 15" : "M6 9L12 15 18 9"} | ||
/> | ||
</svg> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ | |
* and hosts it in the component. When the component is selected, it | ||
* drops-down the contentComponent and applies the contentProps. | ||
*/ | ||
import React, { useEffect, useRef, useState } from "react"; | ||
import React, { useCallback, useEffect, useRef, useState } from "react"; | ||
|
||
import { useDidUpdateEffect } from "../hooks/use-did-update-effect"; | ||
import { useKey } from "../hooks/use-key"; | ||
|
@@ -43,7 +43,7 @@ const Dropdown = () => { | |
const [hasFocus, setHasFocus] = useState(false); | ||
const FinalArrow = ArrowRenderer || Arrow; | ||
|
||
const wrapper: any = useRef(); | ||
const wrapperRef = useRef<HTMLDivElement>(null); | ||
|
||
useDidUpdateEffect(() => { | ||
onMenuToggle && onMenuToggle(expanded); | ||
|
@@ -54,7 +54,7 @@ const Dropdown = () => { | |
setIsInternalExpand(false); | ||
setExpanded(isOpen); | ||
} | ||
}, [isOpen]); | ||
}, [isOpen, defaultIsOpen]); | ||
|
||
const handleKeyDown = (e) => { | ||
// allows space and enter when focused on input/button | ||
|
@@ -68,7 +68,7 @@ const Dropdown = () => { | |
if (isInternalExpand) { | ||
if (e.code === KEY.ESCAPE) { | ||
setExpanded(false); | ||
wrapper?.current?.focus(); | ||
wrapperRef?.current?.focus(); | ||
} else { | ||
setExpanded(true); | ||
} | ||
|
@@ -77,11 +77,11 @@ const Dropdown = () => { | |
}; | ||
|
||
useKey([KEY.ENTER, KEY.ARROW_DOWN, KEY.SPACE, KEY.ESCAPE], handleKeyDown, { | ||
target: wrapper, | ||
target: wrapperRef, | ||
}); | ||
|
||
const handleHover = (iexpanded: boolean) => { | ||
isInternalExpand && shouldToggleOnHover && setExpanded(iexpanded); | ||
const handleHover = (isExpanded: boolean) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed spelling mistake |
||
isInternalExpand && shouldToggleOnHover && setExpanded(isExpanded); | ||
}; | ||
|
||
const handleFocus = () => !hasFocus && setHasFocus(true); | ||
|
@@ -101,11 +101,14 @@ const Dropdown = () => { | |
isInternalExpand && setExpanded(isLoading || disabled ? false : !expanded); | ||
}; | ||
|
||
const handleClearSelected = (e) => { | ||
e.stopPropagation(); | ||
onChange([]); | ||
isInternalExpand && setExpanded(false); | ||
}; | ||
const handleClearSelected = useCallback( | ||
(e) => { | ||
e.stopPropagation(); | ||
onChange([]); | ||
isInternalExpand && setExpanded(false); | ||
}, | ||
[onChange, isInternalExpand] | ||
); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will avoid creating a new function on every render, which can improve performance |
||
return ( | ||
<div | ||
|
@@ -115,7 +118,7 @@ const Dropdown = () => { | |
aria-expanded={expanded} | ||
aria-readonly={true} | ||
aria-disabled={disabled} | ||
ref={wrapper} | ||
ref={wrapperRef} | ||
onFocus={handleFocus} | ||
onBlur={handleBlur} | ||
onMouseEnter={handleMouseEnter} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import React from "react"; | ||
import React, { useMemo } from "react"; | ||
|
||
import { useMultiSelect } from "../hooks/use-multi-select"; | ||
|
||
|
@@ -7,16 +7,23 @@ export const DropdownHeader = () => { | |
|
||
const noneSelected = value.length === 0; | ||
const allSelected = value.length === options.length; | ||
const customText = valueRenderer && valueRenderer(value, options); | ||
const selectedText = valueRenderer | ||
? valueRenderer(value, options) | ||
: "Text Undefined"; | ||
|
||
const getSelectedText = () => value.map((s) => s.label).join(", "); | ||
|
||
return noneSelected ? ( | ||
<span className="gray">{customText || t("selectSomeItems")}</span> | ||
) : ( | ||
<span> | ||
{customText || | ||
(allSelected ? t("allItemsAreSelected") : getSelectedText())} | ||
</span> | ||
const getSelectedText = useMemo( | ||
() => () => value.map((s) => s.label).join(", "), | ||
[value] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using the useMemo hook to memoize the getSelectedText function, to avoid recomputing it on every render. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are you overcomplicating this by returning a function from Could be simply a string returned from useMemo with no invocation |
||
); | ||
|
||
switch (true) { | ||
case noneSelected: | ||
return ( | ||
<span className="gray">{selectedText || t("selectSomeItems")}</span> | ||
); | ||
case allSelected: | ||
return <span>{selectedText || t("allItemsAreSelected")}</span>; | ||
default: | ||
return <span>{selectedText || getSelectedText()}</span>; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ interface IDefaultItemRendererProps { | |
checked: boolean; | ||
option: Option; | ||
disabled?: boolean; | ||
onClick; | ||
onClick: () => void; | ||
} | ||
|
||
const DefaultItemRenderer = ({ | ||
|
@@ -15,16 +15,20 @@ const DefaultItemRenderer = ({ | |
onClick, | ||
disabled, | ||
}: IDefaultItemRendererProps) => ( | ||
<div className={`item-renderer ${disabled ? "disabled" : ""}`}> | ||
<label | ||
className={`item-renderer ${disabled ? "disabled" : ""}`} | ||
htmlFor={option.value} | ||
> | ||
<input | ||
id={option.value} | ||
type="checkbox" | ||
onChange={onClick} | ||
checked={checked} | ||
tabIndex={-1} | ||
disabled={disabled} | ||
/> | ||
<span>{option.label}</span> | ||
</div> | ||
</label> | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
export default DefaultItemRenderer; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. simplify code by using the include method. |
||
itemRenderer={ItemRenderer} | ||
disabled={o.disabled || disabled} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -173,6 +173,18 @@ | |
animation: dash 1.5s ease-in-out infinite; | ||
} | ||
|
||
.rmsc__arrow-svg { | ||
fill: none; | ||
stroke: currentColor; | ||
stroke-width: 2; | ||
} | ||
|
||
.rmsc__cross-svg { | ||
fill: none; | ||
stroke: currentColor; | ||
stroke-width: 2; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style SVG element using CSS value instead of writing hardcoded code. now, the user could override this value if he wants. |
||
@keyframes rotate { | ||
100% { | ||
transform: rotate(360deg); | ||
|
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.
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.