-
Notifications
You must be signed in to change notification settings - Fork 829
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
[EuiDatePickerRange] Respect endDateControl
className
prop
#5329
Conversation
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.
Perfection!! Great spot with the className
fix as well!
@thompsongl Actually, sorry, 1 quick thought - do we want to add a small unit test for this? e.g., it('uses individual EuiDatePicker props', () => {
const component = render(
<EuiDatePickerRange
startDateControl={
<EuiDatePicker popoverPlacement="left" className="hello" />
}
endDateControl={
<EuiDatePicker popoverPlacement="left" className="world" />
}
{...requiredProps}
/>
);
expect(component).toMatchSnapshot();
}); |
Can't argue with adding a test that's copy-paste-able |
…nto 5328-datepicker
Preview documentation changes for this PR: https://eui.elastic.co/pr_5329/ |
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.
Gah I did check the output locally, but apparently still had my override in place |
Yeah sorry, I think I meant specifically we need to check the published CI build. There could be other local issues that don't show up until you view the that version. |
I'm not sure we'd also want them to change the left/right position either. We may need to make this a custom prop asking for top/bottom position that affect both the start and end controls in one prop. |
It looks like changing left/right is what they want, not top/bottom: elastic/kibana#116471 Maybe there's a way to expand the <EuiDatePickerRange isCustom={{popoverPlacement: true}} /> // maybe `false` makes more sense? Resulting in |
I've asked for clarification: elastic/kibana#116471 (comment) |
Ugh, btw, none of this would be a problem if the popover was just one of ours inside of a portal. |
Ughhhh you're right |
Found her a workaround. I think the ideal solution is to get ours rendering in a popover. We can do that by splitting the input from the calendar and putting in inline version in a popover. We do something similar in EuiSuperDatePicker. I started to explore the option in this codesandbox. |
Thanks, @cchaos! I had the idea to use I'm going to scale this PR back to just the className fix and we can open a new PR with either your idea or a broader component reorg. |
endDateControl
popoverPlacement
propendDateControl
className
prop
Preview documentation changes for this PR: https://eui.elastic.co/pr_5329/ |
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!
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.
👍 Maybe update the PR description too?
Summary
Use the
className
prop from the correct element (endDateControl
)Checklist
Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examplesAdded or updated jest and cypress tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately