-
Notifications
You must be signed in to change notification settings - Fork 842
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
Global date picker #1026
Global date picker #1026
Conversation
There are a few issues in IE, that I'm working on fixing. |
Ok IE is fixed, this is ready for review. |
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.
LGTM; pulled and ran the example
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.
Very nice. Some small stuff. Think we want to be really clear about how we label this. "Pattern" I think lego bricks together. Think we wanna be a little more scary about it up front.
Looks great. 🥇
<EuiSpacer /> | ||
<p> | ||
This pattern showcases how to setup the components for Kibana's | ||
global date picker. None of the functionality is actually hooked up. |
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 think you might want to be a little more clear here that this is a visual pattern only. I know EuiCallOut
is uglier, but we need to be really blunt that this isn't for use. I think I'd specifically mention that it utilizes the DatePicker container stuff, and maybe give a line or two about what's broken in the implementation and what would need to be addressed to make it workable.
button={quickSelectButton} | ||
isOpen={this.state.isPopoverOpen} | ||
closePopover={this.closePopover.bind(this)} | ||
anchorPosition="downLeft" |
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.
Adding ownFocus
here will trap the keyboard properly (and allow you to ESC your way out immediately)
<EuiFormControlLayout | ||
prepend={quickSelectPopover} | ||
> | ||
<EuiDatePickerRange |
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.
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.
The only thing I can do is reduce the side paddings of inputs or increase the max-width of this component. The latter doesn't really help all that much because it also depends on where it's located and what size the browser width is at.
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.
No worries.
@@ -24,7 +27,7 @@ | |||
// On mobile we force them to stack and act the same. | |||
@include euiBreakpoint('xs','s') { | |||
.euiFlexGroup--responsive > .euiFlexItem, | |||
.euiFlexGrid > .euiFlexItem { | |||
.euiFlexGrid--responsive > .euiFlexItem { |
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.
Funny we didn't add this till now. 👍
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.
There's a lot of responsive stuff that we just assume that would be nice to allow for more customization.
@@ -33,6 +35,10 @@ | |||
visibility: hidden; /* 2 */ | |||
transform: translateY(0) translateX(0) translateZ(0); /* 2 */ | |||
|
|||
> * { |
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.
😈 Makes sense, but this won't save things completely. Prolly decent default though. Think it's OK for us to expect people to write their own media queries if they are overstuffing popovers.
@@ -44,7 +44,7 @@ | |||
"prop-types": "^15.6.0", | |||
"react-ace": "^5.5.0", | |||
"react-color": "^2.13.8", | |||
"react-datepicker": "v1.4.1", | |||
"react-datepicker": "v1.5.0", |
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.
Could you find a changelog? I did some clicking around and stuff to make sure everything still worked proper, but I couldn't find a list of changes in his lib.
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 just looked here Hacker0x01/react-datepicker@v1.4.1...master up until the 1.5.0. commit
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.
^^ Ran through this and did some pretty deep manual testing to make sure everything still worked the way we expect.
*/ | ||
.euiPopover__panel { | ||
position: absolute; | ||
z-index: $euiZContentMenu; | ||
min-width: $euiButtonMinWidth; /* 1 */ | ||
max-width: calc(100vw - #{$euiSizeXL}); /* 3 */ |
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.
Is this for mobile? Assume content would just break if it needed a width that was set in the desktop version. This seems like a good default though.
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.
Yes, this is for mobile. Just an extra security blanket that makes sure popovers don't go outside the browser width.
* Evenly stretches each tab to fill the | ||
* horizontal space | ||
*/ | ||
expand: PropTypes.bool, |
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.
This is a great addition. Nice implementation.
Oh, and don't forget a changelog. |
- Uses `calendarContainer` from react-datepicker v1.5.0
70d817c
to
7f1ceee
Compare
This PR sets up the pattern (building blocks) for the Global Date Picker based off of these mocks.
Using react-datepicker
I had to bump the react-datepicker version to 1.5.0 so I could have access to the new
calendarContainer
prop. I looked through their commits from the previous version to this, and it seems no other major changes were added.The only issue with building it out this way, is that the datepicker's input does some force focus stuff so the form controls in the popper are hard to interact with as the input keeps stealing focus.
I'm going to leave the rest up to the engineer as the style and display of the pattern are there.
A couple other components had to be manipulated including:
EuiFlexGrid
: Now accepts aresponsive
property likeEuiFlexGroup
does.EuiTabs
andEuiTabbedContent
: Now accepts anexpand
property to expand all the child tabs to fill up the horizontal space. I purposefully did not include and extra doc example, but it is commented in the the props.