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

[Maps] use EuiSuperDatePicker in QueryBar instead of Angular timepicker in Top Nav #29235

Merged
merged 29 commits into from
Jan 31, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 24, 2019

This PR breaks out the part of #29204 that can be backported to 6.7. This includes updating QueryBar with the prop showDatePicker so applications can turn on EuiSuperDatePicker.

screen shot 2019-01-23 at 7 24 31 pm

* replace kbnTimepicker directive with EuiSuperDatePicker

* remove kbnTimepicker directive

* remove bootstrap datepicker

* embed timepicker in query bar

* flesh out date picker in query bar for maps app

* wire up refresh config
@nreese nreese added v7.0.0 [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 labels Jan 24, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -533,6 +604,7 @@ export class QueryBarUI extends Component<Props, State> {
</div>
</EuiOutsideClickDetector>
</EuiFlexItem>
{this.renderDatePicker()}
<EuiFlexItem grow={false}>
<EuiButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the EuiSuperDatePicker's update/refresh button and already includes all the states you need including icons and tooltip? Otherwise, I'd ask that we port over that button to here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because then there would be 2 update buttons - one from EuiSuperDatePicker and one from QueryBar. Linking the DatePicker changes to the QueryBar update button allows users to set the query and time range and apply the changes in a single action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but I thought the EuiSuperDatePicker's update button was created in a way that you could tap into it's state and onClick functions so that it could be used instead of the one that's already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The EuiSuperDatePicker's update button just works on the start/end props, not outside state like query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So its the other way around, if you want update logic to include other stuff then you set showUpdateButton to false and add your own button/logic in the wrapping component

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then we'll want to convert the EuiSuperDatePicker's renderUpdateButton function to an actual EUI component so that it can replace the query bar's update button. That can be an 'eventually' but the states and tooltips that are there for the date picker should be there when the date picker is in conjunction with the query bar.

@nreese
Copy link
Contributor Author

nreese commented Jan 25, 2019

@cchaos I have updated the PR to use EuiSuperDatePicker. Make sure to re-run yarn kbn bootstrap.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@cchaos
Copy link
Contributor

cchaos commented Jan 25, 2019

Great! Let me fix up some of the sizing and send you a PR.

@cchaos
Copy link
Contributor

cchaos commented Jan 25, 2019

PR4U: nreese#29

@nreese nreese requested a review from a team as a code owner January 25, 2019 18:59
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Jan 30, 2019

@lukasolson This is ready for another look. Those problems you were seeing with the date popovers closing are cleared up with EUI 6.7.4. Run yarn kbn clean and yarn kbn bootstrap to ensure you have the latest EUI.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM, there are a couple of minor things that you can feel free to ignore

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Jan 30, 2019

jenkins, test this

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v6.7.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants