Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Exchange defaultProps for default parameters in function components #1226

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Exchange defaultProps for default parameters in function components #1226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
643aa8b
remove defaultProps for CheckboxTree
asizemore Oct 9, 2024
424d980
remove defaultProps from SelectTree
asizemore Oct 11, 2024
081fb80
remove defaultProps from FieldFilter
asizemore Oct 11, 2024
1fe8bc7
create a shared defaultCheckboxTreeProps
asizemore Oct 11, 2024
52768af
remove defaultProps from ServerSideAttributeFilter
asizemore Oct 11, 2024
cb4a3e4
remove more defaultProps
asizemore Oct 11, 2024
b8bced1
fix overlooked default in ServerSideAttributeFilter
asizemore Oct 11, 2024
71d6502
Merge branch 'main' into debt-1218-remove-defaultProps
asizemore Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 10 additions & 64 deletions packages/libs/coreui/src/components/inputs/SelectTree/SelectTree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,18 @@ export interface SelectTreeProps<T> extends CheckboxTreeProps<T> {
}

function SelectTree<T>(props: SelectTreeProps<T>) {
const {
shouldCloseOnSelection,
wrapPopover,
currentList,
selectedList = [],
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this. Isn't selectedList an option prop for CheckboxTree?

} = props;

const [buttonDisplayContent, setButtonDisplayContent] = useState<ReactNode>(
props.currentList && props.currentList.length
? props.currentList.join(', ')
currentList && currentList.length
? currentList.join(', ')
: props.buttonDisplayContent
);
const { selectedList, shouldCloseOnSelection, wrapPopover } = props;

/** Used as a hack to "auto close" the popover when shouldCloseOnSelection is true */
const [key, setKey] = useState('');
Expand All @@ -35,47 +41,7 @@ function SelectTree<T>(props: SelectTreeProps<T>) {
);
};

const checkboxTree = (
<CheckboxTree
tree={props.tree}
getNodeId={props.getNodeId}
getNodeChildren={props.getNodeChildren}
onExpansionChange={props.onExpansionChange}
shouldExpandDescendantsWithOneChild={
props.shouldExpandDescendantsWithOneChild
}
shouldExpandOnClick={props.shouldExpandOnClick}
showRoot={props.showRoot}
renderNode={props.renderNode}
expandedList={props.expandedList}
isSelectable={props.isSelectable}
selectedList={selectedList}
filteredList={props.filteredList}
customCheckboxes={props.customCheckboxes}
isMultiPick={props.isMultiPick}
name={props.name}
onSelectionChange={props.onSelectionChange}
currentList={props.currentList}
defaultList={props.defaultList}
isSearchable={props.isSearchable}
autoFocusSearchBox={props.autoFocusSearchBox}
showSearchBox={props.showSearchBox}
searchBoxPlaceholder={props.searchBoxPlaceholder}
searchIconName={props.searchIconName}
searchBoxHelp={props.searchBoxHelp}
searchTerm={props.searchTerm}
onSearchTermChange={props.onSearchTermChange}
searchPredicate={props.searchPredicate}
renderNoResults={props.renderNoResults}
linksPosition={props.linksPosition}
additionalActions={props.additionalActions}
additionalFilters={props.additionalFilters}
isAdditionalFilterApplied={props.isAdditionalFilterApplied}
wrapTreeSection={props.wrapTreeSection}
styleOverrides={props.styleOverrides}
customTreeNodeCssSelectors={props.customTreeNodeCssSelectors}
/>
);
const checkboxTree = <CheckboxTree {...props} selectedList={selectedList} />;

return (
<PopoverButton
Expand All @@ -89,25 +55,5 @@ function SelectTree<T>(props: SelectTreeProps<T>) {
);
}

const defaultProps = {
showRoot: false,
expandedList: null,
isSelectable: false,
selectedList: [],
customCheckboxes: {},
isMultiPick: true,
onSelectionChange: () => {},
isSearchable: false,
showSearchBox: true,
searchBoxPlaceholder: 'Search...',
searchBoxHelp: '',
searchTerm: '',
onSearchTermChange: () => {},
searchPredicate: () => true,
linksPosition: LinksPosition.Both,
isDisabled: false,
};

SelectTree.defaultProps = defaultProps;
SelectTree.LinkPlacement = LinksPosition;
export default SelectTree;
104 changes: 55 additions & 49 deletions packages/libs/coreui/src/components/inputs/checkboxes/CheckboxTree/CheckboxTree.tsx
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

It occurred to me that you could do something like this in the component:

function CheckboxTree<T>(partialProps: Props<T>) {
  const props = { ...defaultProps, ...partialProps };

  // ...

}

If a prop is defined in partialProps, it's value will be used; otherwise, the value in defaultProps will be used.

Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ export type CheckboxTreeProps<T> = {
isSelectable?: boolean;

/** List of selected nodes as represented by their ids, defaults to [ ] */
selectedList: string[];
selectedList?: string[];

/**
* List of filtered nodes as represented by their ids used to determine isLeafVisible node status.
Expand All @@ -183,7 +183,7 @@ export type CheckboxTreeProps<T> = {

/** Takes array of ids, thus encapsulates:
selectAll, clearAll, selectDefault, selectCurrent (i.e. reset) */
onSelectionChange: ChangeHandler;
onSelectionChange?: ChangeHandler;

/** List of “current” ids, if omitted (undefined or null), then don’t display link */
currentList?: string[];
Expand All @@ -194,7 +194,7 @@ export type CheckboxTreeProps<T> = {
//%%%%%%%%%%% Properties associated with search %%%%%%%%%%%

/** Indicates whether this is a searchable CBT. If so, then show boxes and respect the optional parameters below, also turn off expansion; default to false */
isSearchable: boolean;
isSearchable?: boolean;

/** Indicates if the search box should have autoFocus set to true */
autoFocusSearchBox?: boolean;
Expand All @@ -203,7 +203,7 @@ export type CheckboxTreeProps<T> = {
showSearchBox?: boolean;

/** PlaceHolder text; shown in grey if searchTerm is empty */
searchBoxPlaceholder: string;
searchBoxPlaceholder?: string;

/** Name of icon to show in search box */
searchIconName?: 'search' | 'filter';
Expand All @@ -215,13 +215,13 @@ export type CheckboxTreeProps<T> = {
searchBoxHelp?: string;

/** Current search term; if non-empty, expandability is disabled */
searchTerm: string;
searchTerm?: string;

/** Takes single arg: the new search text. Called when user types into the search box */
onSearchTermChange: (term: string) => void;
onSearchTermChange?: (term: string) => void;

/** Takes (node, searchTerms) and returns boolean. searchTerms is a list of query terms, parsed from the original input string. This function returns a boolean indicating if a node matches search criteria and should be shown */
searchPredicate: (node: T, terms: string[]) => boolean;
searchPredicate?: (node: T, terms: string[]) => boolean;

renderNoResults?: (searchTerm: string, tree: T) => React.ReactNode;

Expand All @@ -247,6 +247,29 @@ export type CheckboxTreeProps<T> = {
customTreeNodeCssSelectors?: object;
};

// Default values. Used across multiple functions in this file.
const defaultCheckboxTreeProps = {
showRoot: false,
expandedList: null,
isSelectable: false,
selectedList: [],
customCheckboxes: {},
isMultiPick: true,
onSelectionChange: () => {
/* */
},
isSearchable: false,
showSearchBox: true,
searchBoxPlaceholder: 'Search...',
searchBoxHelp: '',
searchTerm: '',
onSearchTermChange: () => {
/* */
},
searchPredicate: () => true,
linksPosition: LinksPosition.Both,
};

type TreeLinkHandler = MouseEventHandler<HTMLButtonElement>;

type TreeLinksProps = {
Expand Down Expand Up @@ -464,7 +487,7 @@ function applyPropsToStatefulTree<T>(
isSearchable: CheckboxTreeProps<T>['isSearchable'],
isMultiPick: CheckboxTreeProps<T>['isMultiPick'],
searchTerm: CheckboxTreeProps<T>['searchTerm'],
selectedList: CheckboxTreeProps<T>['selectedList'],
selectedList: CheckboxTreeProps<T>['selectedList'] = defaultCheckboxTreeProps.selectedList,
propsExpandedList: CheckboxTreeProps<T>['expandedList'],
isAdditionalFilterApplied: CheckboxTreeProps<T>['isAdditionalFilterApplied'],
isLeafVisible: (id: string) => boolean,
Expand Down Expand Up @@ -590,8 +613,8 @@ function applyPropsToStatefulTree<T>(
*/
function isActiveSearch<T>(
isAdditionalFilterApplied: CheckboxTreeProps<T>['isAdditionalFilterApplied'],
isSearchable: CheckboxTreeProps<T>['isSearchable'],
searchTerm: CheckboxTreeProps<T>['searchTerm']
isSearchable: CheckboxTreeProps<T>['isSearchable'] = defaultCheckboxTreeProps.isSearchable,
searchTerm: CheckboxTreeProps<T>['searchTerm'] = defaultCheckboxTreeProps.searchTerm
) {
return isSearchable && isFiltered(searchTerm, isAdditionalFilterApplied);
}
Expand Down Expand Up @@ -626,12 +649,12 @@ function isFiltered(searchTerm: string, isAdditionalFilterApplied?: boolean) {
*/
function createIsLeafVisible<T>(
tree: CheckboxTreeProps<T>['tree'],
searchTerm: CheckboxTreeProps<T>['searchTerm'],
searchPredicate: CheckboxTreeProps<T>['searchPredicate'],
searchTerm: CheckboxTreeProps<T>['searchTerm'] = defaultCheckboxTreeProps.searchTerm,
searchPredicate: CheckboxTreeProps<T>['searchPredicate'] = defaultCheckboxTreeProps.searchPredicate,
getNodeId: CheckboxTreeProps<T>['getNodeId'],
getNodeChildren: CheckboxTreeProps<T>['getNodeChildren'],
isAdditionalFilterApplied: CheckboxTreeProps<T>['isAdditionalFilterApplied'],
isSearchable: CheckboxTreeProps<T>['isSearchable'],
isSearchable: CheckboxTreeProps<T>['isSearchable'] = defaultCheckboxTreeProps.isSearchable,
filteredList: CheckboxTreeProps<T>['filteredList']
) {
// if not searching, if no additional filters are applied, and if filteredList is undefined, then all nodes are visible
Expand Down Expand Up @@ -696,32 +719,38 @@ function CheckboxTree<T>(props: CheckboxTreeProps<T>) {
tree,
getNodeId,
getNodeChildren,
searchTerm,
selectedList,
searchTerm = '',
selectedList = [],
Copy link
Member

Choose a reason for hiding this comment

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

This will create a new array on each render. Use defaultCheckboxTreeProps.selectedList to prevent that

Suggested change
selectedList = [],
selectedList = defaultCheckboxTreeProps.selectedList,

currentList,
defaultList,
isSearchable,
expandedList = null,
isSearchable = false,
isAdditionalFilterApplied,
name,
shouldExpandDescendantsWithOneChild,
onExpansionChange,
isSelectable,
isMultiPick,
onSelectionChange,
showRoot,
onSelectionChange = () => {
//
},
Comment on lines +732 to +734
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSelectionChange = () => {
//
},
onSelectionChange = defaultCheckboxTreeProps.onSelectionChange,

isSelectable = false,
isMultiPick = true,
showRoot = false,
additionalActions,
linksPosition = LinksPosition.Both,
showSearchBox,
showSearchBox = true,
autoFocusSearchBox,
onSearchTermChange,
searchBoxPlaceholder,
onSearchTermChange = () => {
//
},
Comment on lines +742 to +744
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onSearchTermChange = () => {
//
},
onSearchTermChange = defaultCheckboxTreeProps.onSearchTermChange,

searchBoxPlaceholder = 'Search...',
searchIconName,
searchIconPosition,
searchBoxHelp,
searchBoxHelp = '',
searchPredicate = () => true,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
searchPredicate = () => true,
searchPredicate = defaultCheckboxTreeProps.searchPredicate,

additionalFilters,
wrapTreeSection,
shouldExpandOnClick = true,
customCheckboxes,
customCheckboxes = {},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
customCheckboxes = {},
customCheckboxes = defaultCheckboxTreeProps.customCheckboxes,

renderNoResults,
styleOverrides = {},
customTreeNodeCssSelectors = {},
Expand Down Expand Up @@ -765,7 +794,7 @@ function CheckboxTree<T>(props: CheckboxTreeProps<T>) {
* Creates a function that will handle selection-related tree link clicks
*/
function createSelector(listFetcher: ListFetcher) {
return createLinkHandler(listFetcher, props.onSelectionChange);
return createLinkHandler(listFetcher, onSelectionChange);
}

// define event handlers related to expansion
Expand Down Expand Up @@ -1071,29 +1100,6 @@ function defaultRenderNoResults() {
);
}

const defaultProps = {
showRoot: false,
expandedList: null,
isSelectable: false,
selectedList: [],
customCheckboxes: {},
isMultiPick: true,
onSelectionChange: () => {
/* */
},
isSearchable: false,
showSearchBox: true,
searchBoxPlaceholder: 'Search...',
searchBoxHelp: '',
searchTerm: '',
onSearchTermChange: () => {
/* */
},
searchPredicate: () => true,
linksPosition: LinksPosition.Both,
};

CheckboxTree.defaultProps = defaultProps;
CheckboxTree.LinkPlacement = LinksPosition;
export default CheckboxTree;

Expand Down
12 changes: 5 additions & 7 deletions packages/libs/wdk-client/src/Components/AttributeFilter/FieldFilter.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@ const cx = makeClassNameHelper('field-detail');
* Main interactive filtering interface for a particular field.
*/
function FieldFilter(props) {
const { displayName = 'Items' } = props;

let className = cx('', props.hideFieldPanel && 'fullWidth');

return (
<div className={className}>
{!props.activeField ? (
<EmptyField displayName={props.displayName} />
<EmptyField displayName={displayName} />
) : (
<React.Fragment>
<h3>
Expand Down Expand Up @@ -48,9 +50,9 @@ function FieldFilter(props) {
props.activeFieldState.summary == null &&
props.activeFieldState.leafSummaries == null) ||
props.dataCount == null ? null : isMulti(props.activeField) ? (
<MultiFieldFilter {...props} />
<MultiFieldFilter {...props} displayName={displayName} />
) : (
<SingleFieldFilter {...props} />
<SingleFieldFilter {...props} displayName={displayName} />
)}
</React.Fragment>
)}
Expand Down Expand Up @@ -95,8 +97,4 @@ FieldFilter.propTypes = {
selectByDefault: PropTypes.bool.isRequired,
};

FieldFilter.defaultProps = {
displayName: 'Items',
};

export default FieldFilter;
25 changes: 13 additions & 12 deletions packages/libs/wdk-client/src/Components/AttributeFilter/ServerSideAttributeFilter.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,26 @@ import FieldFilter from '../../Components/AttributeFilter/FieldFilter';
* Filtering UI for server-side filtering.
*/
function ServerSideAttributeFilter(props) {
var { displayName, fieldTree, hideFilterPanel, hideFieldPanel } = props;
const {
fieldTree,
displayName = 'Items',
hideFilterPanel = false,
hideFieldPanel = false,
hideGlobalCounts = false,
selectByDefault = false,
histogramScaleYAxisDefault = true,
histogramTruncateYAxisDefault = false,
} = props;

if (fieldTree == null) {
return <h3>Data is not available for {displayName}.</h3>;
}

return (
<div style={{ overflowX: 'auto', marginRight: '1em' }}>
{hideFilterPanel || <FilterList {...props} />}
{hideFilterPanel || (
<FilterList {...props} hideGlobalCounts={hideGlobalCounts} />
)}

{/* Main selection UI */}
<div className="filters ui-helper-clearfix">
Expand Down Expand Up @@ -79,16 +90,6 @@ ServerSideAttributeFilter.propTypes = {
onRangeScaleChange: PropTypes.func.isRequired,
};

ServerSideAttributeFilter.defaultProps = {
displayName: 'Items',
hideFilterPanel: false,
hideFieldPanel: false,
hideGlobalCounts: false,
selectByDefault: false,
histogramScaleYAxisDefault: true,
histogramTruncateYAxisDefault: false,
};

export default wrappable(ServerSideAttributeFilter);

export function withOptions(options) {
Expand Down
Loading
Loading