-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add affordance to icon selector in docs and fix selection state #6877
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 |
---|---|---|
|
@@ -21,7 +21,7 @@ import { Alignment, Button, Classes, MenuItem } from "@blueprintjs/core"; | |
import type { IconName } from "@blueprintjs/icons"; | ||
import { type ItemRenderer, Select } from "@blueprintjs/select"; | ||
|
||
import { getIconNames, type IconNameOrNone, NONE } from "./iconNames"; | ||
import { getIconNames } from "./iconNames"; | ||
|
||
const ICON_NAMES = getIconNames(); | ||
|
||
|
@@ -35,39 +35,42 @@ export class IconSelect extends React.PureComponent<IconSelectProps> { | |
public render() { | ||
const { disabled, iconName } = this.props; | ||
return ( | ||
<label className={classNames(Classes.LABEL, { [Classes.DISABLED]: disabled })}> | ||
<label className={classNames("icon-select", Classes.LABEL, { [Classes.DISABLED]: disabled })}> | ||
Icon | ||
<Select<IconNameOrNone> | ||
<Select<IconName> | ||
disabled={disabled} | ||
items={ICON_NAMES} | ||
itemPredicate={this.filterIconName} | ||
itemRenderer={this.renderIconItem} | ||
noResults={<MenuItem disabled={true} text="No results" />} | ||
placeholder="Start typing to search…" | ||
onItemSelect={this.handleIconChange} | ||
popoverProps={{ minimal: true }} | ||
> | ||
<Button | ||
alignText={Alignment.LEFT} | ||
className={Classes.TEXT_OVERFLOW_ELLIPSIS} | ||
textClassName={Classes.TEXT_OVERFLOW_ELLIPSIS} | ||
disabled={disabled} | ||
fill={true} | ||
icon={iconName} | ||
text={iconName || NONE} | ||
text={iconName || "None"} | ||
rightIcon="caret-down" | ||
/> | ||
</Select> | ||
</label> | ||
); | ||
} | ||
|
||
private renderIconItem: ItemRenderer<IconName | typeof NONE> = (icon, { handleClick, handleFocus, modifiers }) => { | ||
private renderIconItem: ItemRenderer<IconName | undefined> = (icon, { handleClick, handleFocus, modifiers }) => { | ||
if (!modifiers.matchesPredicate) { | ||
return null; | ||
} | ||
return ( | ||
<MenuItem | ||
selected={modifiers.active} | ||
icon={icon === NONE ? undefined : icon} | ||
roleStructure="listoption" | ||
active={modifiers.active} | ||
selected={this.props.iconName === icon} | ||
icon={icon} | ||
key={icon} | ||
onClick={handleClick} | ||
onFocus={handleFocus} | ||
|
@@ -76,15 +79,14 @@ export class IconSelect extends React.PureComponent<IconSelectProps> { | |
); | ||
}; | ||
|
||
private filterIconName = (query: string, iconName: IconName | typeof NONE) => { | ||
if (iconName === NONE) { | ||
return true; | ||
} | ||
private filterIconName = (query: string, iconName: IconName | undefined) => { | ||
if (query === "") { | ||
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 prevents loading the full list on empty query - there's over 600 icons and it's a bit laggy if showing all... and doesn't quite feel worth bringing in a virtualized list for 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. I was a bit surprised it was starting to struggle with just 600 items, in case this raises any concerns |
||
return iconName === this.props.iconName; | ||
} | ||
return iconName.toLowerCase().indexOf(query.toLowerCase()) >= 0; | ||
}; | ||
|
||
private handleIconChange = (icon: IconNameOrNone) => this.props.onChange(icon === NONE ? undefined : icon); | ||
private handleIconChange = (icon: IconName) => { | ||
this.props.onChange(icon === this.props.iconName ? undefined : icon); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,14 @@ export interface SelectProps<T> extends ListItemsProps<T>, SelectPopoverProps { | |
*/ | ||
menuProps?: React.HTMLAttributes<HTMLUListElement>; | ||
|
||
/** | ||
* A placeholder string passed to the filter text input. | ||
* Applicable only when `filterable` is `true`. | ||
* | ||
* @default "Filter..." | ||
*/ | ||
placeholder?: string; | ||
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. calling out this small new feature. this has some potential to be used inappropriately, but I don't think "Filter..." is always the best indication here particularly since the user is responsible for defining the match predicate and may not strictly implement a filter 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. I agree just "Filter..." may not be that valuable to a user. I also think we'd want this to be customizable for translations. I'm actually surprised a hard-coded English string has survived this long in Blueprint without someone flagging that a translation is necessary. Thanks for adding! |
||
|
||
/** | ||
* Whether the active item should be reset to the first matching item _when | ||
* the popover closes_. The query will also be reset to the empty string. | ||
|
@@ -159,6 +167,7 @@ export class Select<T> extends AbstractPureComponent<SelectProps<T>, SelectState | |
filterable = true, | ||
disabled = false, | ||
inputProps = {}, | ||
placeholder = "Filter...", | ||
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. I used an ellipsis in |
||
popoverContentProps = {}, | ||
popoverProps = {}, | ||
popoverRef, | ||
|
@@ -168,7 +177,7 @@ export class Select<T> extends AbstractPureComponent<SelectProps<T>, SelectState | |
<InputGroup | ||
aria-autocomplete="list" | ||
leftIcon={<Search />} | ||
placeholder="Filter..." | ||
placeholder={placeholder} | ||
rightElement={this.maybeRenderClearButton(listProps.query)} | ||
{...inputProps} | ||
inputRef={this.handleInputRef} | ||
|
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.
Thank you for adding this comment!
This prop being a no-op depending on
roleStructure
would have surprised me too, especially since"listoption"
isn't the default.