-
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
[Select] Add resetOnQuery prop #2894
Conversation
Thanks for your interest in palantir/blueprint, @switz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
53a3e18
to
877074b
Compare
877074b
to
c5b6367
Compare
c5b6367
to
b1bee57
Compare
@switz 😱 this is awesome! will review more carefully soon, just so excited to see this feature implemented!!! |
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.
code looks great, nice tests 👍
just want to discuss the prop name real quick 😄
* entered. The first item will be made active. | ||
* @default false | ||
*/ | ||
resetActiveItemOnQuery?: boolean; |
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.
ok so I see why you called it resetActiveItemOnQuery
instead of resetOnQuery
, but i kinda feel like the mere fact that it has query
in the name suggests it won't reset the query on query. we can add a note in the prop docs to clarify. thoughts?
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.
Yeah, I don't love the name either but felt it was clearer than resetOnQuery
. I'm open to a better name and am happy to expand the docs a bit.
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.
Any thoughts on true
by default?
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.
let's go with resetOnQuery
and see how it feels. i like the consistency.
i'm definitely inclined towards true
by default -- it's the behavior i've been trying to implement all along. question is if it noticeably breaks common use cases?
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.
Ok, I think that prop name is a bit unclear, but will push up a build with that change & defaulting to true
. Playing with the demo a bit feels good -- I don't think that it would break any common use cases.
efeefd4
to
c9b41dd
Compare
c9b41dd
to
b8fa612
Compare
Switch prop name from resetActiveItemOnQuery to resetOnQuery and default prop to truePreview: documentation | landing | table |
/** | ||
* Whether the active selection should be reset when a query is entered. | ||
* If you search and your current active item falls out of scope, | ||
* the first matching item will become the active selection. |
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.
@switz this second sentence is not accurate... changing the query always resets to first active item. did you intend it to only reset if the active item falls out of scope?
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.
That's a great point -- I'm not sure which is better to be honest. I can update the docs in the meantime.
Match documentation to current functionalityPreview: documentation | landing | table |
@@ -100,8 +100,7 @@ export interface IListItemsProps<T> extends IProps { | |||
|
|||
/** | |||
* Whether the active selection should be reset when a query is entered. | |||
* If you search and your current active item falls out of scope, | |||
* the first matching item will become the active selection. | |||
* As you query, the first matching item will become the active selection. |
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.
"Whether the active item should be reset to the first matching item every time the query changes."
simple, precise.
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.
oops, not ready. move the prop default to the lowest level and edit the docs.
isOpen, | ||
inputProps, | ||
overlayProps, | ||
resetOnQuery = true, |
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.
hmm actually this default does not belong here, it should be in QueryList
itself so it applies to all components. you can revert this change as it was already passed through the spread.
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.
I put it there because queryList doesn't have a default props (from what I could tell). Do you want me to add one or can you point me to where it's located?
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.
it's only used the one time in QueryList so you could just modify that method to do the deconstruct-default like you have here.
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.
Unless there's some es6 fanciness that I don't know. If I just handle it in that method I have to do this:
public setQuery(query: string, resetActiveItem?: boolean) {
const { resetOnQuery } = this.props;
// only override resetActiveItem if param is undefined
if (typeof resetActiveItem === 'undefined') {
// default to true if resetOnQuery prop is not defined
if (typeof resetOnQuery === 'undefined') {
resetActiveItem = true;
}
else {
resetActiveItem = resetOnQuery;
}
}
// ...rest of fn
}
so I think I'll add a public static defaultProps
to the component
public static defaultProps = {
resetOnQuery: true,
};
// ...later on...
public setQuery(query: string, resetActiveItem = this.props.resetOnQuery) {
@@ -68,7 +68,7 @@ export class MultiSelect<T> extends React.PureComponent<IMultiSelectProps<T>, IM | |||
|
|||
public render() { | |||
// omit props specific to this component, spread the rest. | |||
const { openOnKeyDown, popoverProps, tagInputProps, ...restProps } = this.props; | |||
const { openOnKeyDown, popoverProps, tagInputProps, resetOnQuery = true, ...restProps } = this.props; |
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.
nope, in QueryList
please
@@ -87,14 +87,15 @@ export class Select<T> extends React.PureComponent<ISelectProps<T>, ISelectState | |||
|
|||
public render() { | |||
// omit props specific to this component, spread the rest. | |||
const { filterable, inputProps, popoverProps, ...restProps } = this.props; | |||
const { filterable, inputProps, popoverProps, resetOnQuery = true, ...restProps } = this.props; |
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.
in QueryList
@@ -60,6 +60,7 @@ export class Suggest<T> extends React.PureComponent<ISuggestProps<T>, ISuggestSt | |||
public static defaultProps = { | |||
closeOnSelect: true, | |||
openOnKeyDown: false, | |||
resetOnQuery: true, |
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.
in QueryList
* @default true | ||
*/ | ||
resetOnQuery?: boolean; | ||
|
||
/** | ||
* Whether the querying state should be reset to initial when an item is | ||
* selected (immediately before `onItemSelect` is invoked). The query will |
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.
Whether the active item should be reset to the first matching item when an item is selected.
The query will also be reset to the empty string.
and please make this same change in resetOnClose
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.
Not sure I follow -- you want that to replace the docs for resetOnSelect
(in listItemProps.ts) & resetOnClose
(in select.tsx)?
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.
that's the goal. i can do it if you're not comfortable
Move default prop down to the lowest level, queryListPreview: documentation | landing | table |
bacfeea
to
166ab14
Compare
Move default prop down to the lowest level, queryListPreview: documentation | landing | table |
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.
👏 great work @switz! question about the tests.
@@ -36,7 +36,7 @@ export function selectComponentSuite<P extends IListItemsProps<IFilm>, S>( | |||
|
|||
describe("common behavior", () => { | |||
it("itemRenderer is called for each child", () => { | |||
const wrapper = render(testProps); | |||
const wrapper = render({ ...testProps, resetOnQuery: false }); |
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.
why was this necessary? does setting the query prop cause an extra re-render?
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.
yeah it re-renders the items
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.
opened a followup PR that resolves this... check it #2916
* Fix tabs querySelector on jsdom (palantir#2761) * match event handler prop name with code and docs (palantir#2760) * matched icons with the original design (palantir#2780) * [Popover] captureDismiss=false by default (palantir#2776) * disable captureDismiss by default as it breaks links * fix tests * redirect old v1, v2 sites to new URLs (palantir#2773) * Remove `HACKHACK` (palantir#2790) BP requires `"@types/react": "^16.0.34"` and it is pinned by `yarn.lock`. For this version of typings, it is provided an overload, then the `HACKHACK` does not persist. ```ts function createElement<P>( type: SFC<P> | ComponentClass<P> | string, props?: Attributes & P, ...children: ReactNode[]): ReactElement<P>; ``` See issue: palantir#2785 * [RadioGroup] [HtmlSelect] options support className and disabled (palantir#2783) * Fix: <RadioGroup> passes custom className to options (+ test) * Fix: <HtmlSelect> passes custom className to options (+ test) * HTMLSelect supports disabled * test all options props * remove className test * [DatePicker] ❤️ (palantir#2789) * tests for DateUtils.getDateTime() * replace setStateWithValueIfUncontrolled with setState & updateValue helper refactor handlers to reduce let vars and clarify logic * refactor constructor into helper functions * merge disabled checks * fix test for initial state * massage imports * little bug fixes - ignoring next month change when null - correct day calculation - comments! * bump react-day-picker * areSameDay calls areSameMonth * reduce nesting * fix tests * Add Popover support for "auto-start" and "auto-end" (palantir#2772) * palantir#2770: Popover now accepts 'auto-start' and 'auto-end' positions * Update example * Update docs * Oops, undo unintended changes to example * [Button] Utils.isEmptyReactNode solves icon regression (palantir#2775) * Utils.isEmptyReactNode solves button regression * naming and test * rename in tests * Publish - @blueprintjs/core@3.3.0 * updated Sketch file (palantir#2813) * [TagInput] On paste, don't tag-ify text if no separator included (palantir#2804) * [TagInput] Leave a delimiter-less value in the input on paste * Update tests * Update docs * [DateRangePicker] all tests use enzyme (palantir#2793) * DRP tests use enzyme everywhere with a cool harness * name clash * fix tests in React 15 by using accessor to find latest element when needed * [new] Divider component (palantir#2854) * add Divider component * example * docs edits * replace all modifiers with borders * ignore coverage * remove fill from example * english is hard * [DatePicker] reuse existing components in caption (palantir#2792) * HTMLSelect: add iconProps, fix dark icon color * use HTMLSelect in caption massive reduction in styles * DatePickerNavbar renders prev/next Buttons use Button for another reduction in styles * cache month widths * DatePicker uses single handleMonthChange for all DayPicker events caption (month/year select) and navbar (prev/next buttons) now all use the same logic for changes! * add DPNavbar to DRP * fix & refactor caption & DP tests assertSelectedDays() helper in DP tests replaces getSelectedDays() * add $datepicker-padding variable * adjust paddings to use standard buttons in navbar * month icon won't exceed select bounds * dateinput tests * renames * replace borders with Divider elements also remove all negative margins * copyright, test helper * use Divider component * fix DRP tests * adjust month select icon position * margin only on caption * fix R15 tests * [DatePicker] time support: timePrecision & timePickerProps (palantir#2856) * add TimePicker props right in DatePicker! * getDateTime() to merge date and time * remove "none" from allowed timePrecision * add style for TP in DP * refactors to MomentDate and PrecisionSelect to support time * add PrecisionSelect to DP and DRP examples * import types, update styles (no divider) * revert some example changes * check null in caption * revert DRP example change * DateInput only renders DatePicker * deprecate DateTimePicker * fix dateinput test * tests for time! * strict boolean, no only * top margin on timepicker * refactors to reduce some complexity (palantir#2858) * [DateRangePicker] Shortcuts component and renderCalendars method (palantir#2859) * Shortcuts component and renderCalendars method to greatly simplify render() * bind handleNextState * Publish - @blueprintjs/icons@3.1.0 * remove dependencies section * [table] fix invisible table menu icon (palantir#2866) Fixes palantir#2865 * [Spinner] restore IE support (palantir#2868) * fix Spinner in IE by adding HTML wrapper tag for the animation * fix loading button spinner position * add tagName test * attempt to fix changing value on IE * added latest version (palantir#2862) * Publish - @blueprintjs/core@3.4.0 - @blueprintjs/table@3.1.1 * sketch file updated date * sandbox link in readme * remove quotes on $ns variable value (palantir#2881) * [Icon] render HTML element & tagName prop (palantir#2884) * Icon tagName prop and set `.bp3-icon-{name}` on element * refactor icon styles so svg is child - only render font glyph if the icon element is :empty * icon docs * color prop becomes fill attribute on svg, overrides css colors * fix tests * oops fix icon classes on non-icon elements (like callouts & buttons) * fix text ref (palantir#2888) * Skeleton: fix FF support! (palantir#2887) and refactor styles for simpler keyframes * [OverflowList] fix browser zoom behavior (palantir#2886) * fix OverflowList when zoomed * less magical number * [docs] better version tag styles (palantir#2889) * [docs] better version tag styles * remove obsolete flex override on tag icons * center docs-nav-buttons and key-combo * [Skeleton] Increase animation contrast (palantir#2885) * [Spinner] add additional child element to isolate spinner from parent (palantir#2890) * add spinner-animation element to isolate spinner from parent * comments about elements * Publish - @blueprintjs/core@3.5.0 * [Icon] revert to inline-block (palantir#2896) * clean up comments * take no chances * revert to inline-block and set block on svg instead of relying on flex child fixes all noted regressions * Publish - @blueprintjs/core@3.5.1 * Incorrect argument name (palantir#2898) Copied this over and realized that the argument should not be item but rather film. * [Suggest] Added selectedItem prop (palantir#2874) * Added selected item prop on the ISuggestProps def. * Init the Suggest state with the selected item prop. * Fixed Suggest support for controlled mode. * Fixed a tslint coma issue. * Made the state the only source of truth, added tests. * Added more tests, improved controlled mode support Now the Suggest does not update its underlying state in controlled mode, just like the EditableText component. * Added support for controlled empty selection. * prop docs * whitespace * [TagInput] Use $input-padding-horizontal when empty for consistency with <InputGroup> (palantir#2900) * Use -padding-horizontal in empty <TagInput> * Remove -empty class, use pure CSS approach * Add left-icon toggle to example * 🔧 switch to tree-sitter-typescript (palantir#2908) * switch to tree-sitter-typescript, move syntax pkgs to docs-data * update syntax tokens * 🔧 switch to circle-github-bot (palantir#2907) * add circle-github-bot * new preview script * restore cache in circle job * delete old scripts * [timepicker] Fix allowing to type time that exceeds time bounds (palantir#2795) * [DateRangePicker] support time selection (palantir#2895) * add time selection unit tests * add maybeRender placeholder * add wrapper div for calendars + time * add time precision to DRP example * reorder function * DateRangePicker now has time selection field * Updated DateRangePicker example to show time when precision is selected * Updated props to include timePickerProps and fixed/cleaned up tests for DateRangePicker * renamed timepicker classes appropiately, and code cleanup * updated momentTime to pass props * clicking on a shortcut doesn't change the time * removed only from describe in tests * removed unneeded CSS property and added test for making sure that time is preserved when un-selecting / reselecting date * Um/fix collapse animation (palantir#2911) * fixes collapse opening animation on first open * fix documentation * Add condensed property to HTML tables (palantir#2904) * Add condensed property to HTML tables * Move description to small instead of condensed * Deprecate small property on HTML tables * comment format * move collapse animation state docs onto the enum values, CLOSING_END -> CLOSING (palantir#2914) * bump sass-inline-svg (palantir#2915) * [docs] Modifiers & update DTP deprecation (palantir#2909) * add modifiers docs * update DTP deprecation notice * back to present tense since we're ready to ship * [Switch] fix switch styles variables for dark theme (palantir#2912) * [Select] Add resetOnQuery prop (palantir#2894) * [Select] Add resetActiveItemOnQuery prop * Switch prop name from resetActiveItemOnQuery to resetOnQuery and default prop to true * Match documentation to current functionality * Move default prop down to the lowest level, queryList
Fixes #2867
Checklist
Changes proposed in this pull request:
Adds a prop called
resetActiveItemOnQuery
which will update a Select/QueryList's activeItem while querying.This feels to me like it should default to
true
, but I'll defer to you.Reviewers should focus on:
Set
resetActiveItemOnQuery=true
and query for "Forrest". You'll see the first item in the list become active as soon as the existingactiveItem
falls out of scope.Screenshot