DateInput popoverProps #978
Conversation
no default prop value; assign it when deconstructingPreview: documentation |
packages/datetime/src/dateInput.tsx
Outdated
| */ | ||
| popoverPosition?: Position; | ||
|
|
||
| /** | ||
| * Props to pass to the `Popover`. Note that `content` cannot be changed. | ||
| */ | ||
| popoverProps?: Partial<IPopoverProps>; |
There was a problem hiding this comment.
this accepts almost any value because it's equal to {}. you should at least combine it with & object to limit it to non-primitive types.
packages/datetime/src/dateInput.tsx
Outdated
| @@ -174,6 +181,7 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |||
| {...sharedProps} | |||
| timePickerProps={{ precision: this.props.timePrecision }} | |||
| />; | |||
| const { popoverProps = {} } = this.props; | |||
There was a problem hiding this comment.
why not use defaultProps?
There was a problem hiding this comment.
see #977 (comment)
There was a problem hiding this comment.
@giladgray Minor thoughts/requests but overall looks good. Will wait for answers before approving.
packages/datetime/src/dateInput.tsx
Outdated
| {...popoverProps} | ||
| content={popoverContent} | ||
| onClose={this.handleClosePopover} | ||
| popoverClassName={classNames("pt-dateinput-popover", popoverProps.popoverClassName)} |
There was a problem hiding this comment.
@giladgray is it still possible to add custom classes to the popover via popoverProps? We should allow for that in general.
There was a problem hiding this comment.
if i'm not mistaken, that is the purpose of this line (and why it comes after the spread)
There was a problem hiding this comment.
there's also popoverProps.className which is covered by spread
packages/datetime/src/dateInput.tsx
Outdated
| @@ -182,13 +190,14 @@ export class DateInput extends AbstractComponent<IDateInputProps, IDateInputStat | |||
| return ( | |||
| <Popover | |||
| autoFocus={false} | |||
| content={popoverContent} | |||
| enforceFocus={false} | |||
There was a problem hiding this comment.
A couple of these should not be overridable based on my experience with #940, particularly autoFocus and enforceFocus. Led to really weird UX.
There was a problem hiding this comment.
truth, thanks for tip
| assert.strictEqual(popover.prop("inline"), true); | ||
| assert.strictEqual(popover.prop("position"), Position.TOP); | ||
| assert.isTrue(popoverWillOpen.calledOnce); | ||
| }); |
There was a problem hiding this comment.
I'd also like to see some tests asserting which props are not overridable. See https://github.com/palantir/blueprint/pull/940/files, e.g.
| @@ -100,10 +101,16 @@ export interface IDateInputProps extends IDatePickerBaseProps, IProps { | |||
| /** | |||
| * The position the date popover should appear in relative to the input box. | |||
| * @default Position.BOTTOM | |||
| * @deprecated since v1.15.0, use `popoverProps.position` | |||
There was a problem hiding this comment.
Note: We should add this message to DateRangeInput in #940 and also NumericInput if we move that component to support IInputProps. (Separate PRs of course)
There was a problem hiding this comment.
i don't understand the request here? DRI does not have any deprecated props, nor does NI.
props extends object, move spread, test locked propsPreview: documentation |
add deprecation warning to DateInputPreview: documentation |
Fixes #560, #974
Changes proposed in this pull request:
popoverPropsprop proxied toPopoverpopoverPosition