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.

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

[DateRangeInput] Add popoverProps prop #940

Merged
merged 6 commits into from
Apr 4, 2017
Merged

Conversation

cmslewis
Copy link
Contributor

@cmslewis cmslewis commented Apr 4, 2017

Addresses #805

Checklist

  • Include tests
  • Update documentation (props table will auto-update with new prop)

Changes proposed in this pull request:

  • Add a popoverProps prop to DateRangeInput to support custom callbacks for popoverWillOpen, onClose, etc.
  • DO NOT allow users to override autoFocus, content, or enforceFocus, because that would lead to sad times w/r/t UX.
  • DO allow for overriding of isOpen and other popover props.

Reviewers should focus on:

  • Does it make sense to allow overriding of isOpen?
  • Should we throw an error or print a warning or otherwise notify the user when certain props they included are being ignored?

@cmslewis cmslewis added this to the 1.14.0 milestone Apr 4, 2017
@cmslewis cmslewis requested review from giladgray and leebyp April 4, 2017 17:37
@@ -6,7 +6,7 @@ The `DateRangeInput` component is a [control group](#core/components/forms/contr

Use this component in forms where the user must enter a date range.

@react-example DateRangeInputExample
@reactExample DateRangeInputExample
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed this docs bug

@blueprint-bot
Copy link

Fix another lint problem

Preview: documentation
Coverage: core | datetime

@@ -124,6 +125,11 @@ export interface IDateRangeInputProps extends IDatePickerBaseProps, IProps {
overlappingDatesMessage?: string;

/**
* The props to pass to the popover.
*/
popoverProps?: IPopoverProps;
Copy link
Contributor

Choose a reason for hiding this comment

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

Partial<IPopoverProps> because content is required...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -276,17 +283,18 @@ export class DateRangeInput extends AbstractComponent<IDateRangeInputProps, IDat
/>
);

// allow custom props for each input group, but pass them in an order
// that guarantees only some props are overridable.
// allow custom props the popover and each input group, but pass them in an order that
Copy link
Contributor

Choose a reason for hiding this comment

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

missing a word?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -124,6 +125,11 @@ export interface IDateRangeInputProps extends IDatePickerBaseProps, IProps {
overlappingDatesMessage?: string;

/**
* The props to pass to the popover.
Copy link
Contributor

Choose a reason for hiding this comment

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

worth mentioning which props are ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

@blueprint-bot
Copy link

Respond to CR feedback

Preview: documentation | landing
Coverage: core | datetime

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

🎉

@cmslewis cmslewis merged commit 2caf687 into master Apr 4, 2017
@cmslewis cmslewis deleted the cl/dri-popover-props branch April 4, 2017 19:02
@giladgray giladgray mentioned this pull request Apr 5, 2017
@cmslewis cmslewis mentioned this pull request Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants