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

Replace angular timepicker with EuiSuperDatePicker #29204

Merged
merged 41 commits into from
Feb 6, 2019

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 23, 2019

Update kbn_top_nav, replacing angular timepicker with EuiSuperDatePicker.

screen shot 2019-01-31 at 8 02 21 am

Example of application that has not disabled the timepicker in kbn-top-nav and has not enabled the timepicker in query-bar
screen shot 2019-01-31 at 8 02 31 am

Example of EuiSuperDatePicker in refresh only mode
screen shot 2019-01-23 at 8 46 38 am

@nreese nreese added Feature:Timepicker Timepicker v7.0.0 Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 23, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@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.

One thing I noticed while playing around with this is that there's no way for apps to subscribe to when you click on the "refresh" button (i.e. when the time range hasn't changed but you still want to refresh your data). It might make sense as part of this PR to move the this.emit('fetch') in timefilter.js outside of the if (areTimePickerValsDifferent(... statement. What do you think?

Other than that, it is looking really great! Love seeing this finally get moved over to EUI and seeing all of those old bootstrap files get removed.

@@ -4,4 +4,4 @@
@import "~ui/styles/bootstrap/bootstrap_dark";

// Components -- waiting on EUI conversion
@import "~ui/timepicker/timepicker";
@import "~ui/filter_bar/filter_bar";
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this is a merge error. Both of these should be removed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, that was a merge mistake. It has been fixed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor Author

nreese commented Feb 5, 2019

jenkins, test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Given time constraints I did a pretty quick review, but the code looks good and everything seems to be working in Chrome. Assuming tests pass, LGTM.

@@ -117,7 +117,7 @@ export function getUiSettingDefaults() {
name: i18n.translate('kbn.advancedSettings.dateFormatTitle', {
defaultMessage: 'Date format',
}),
value: 'MMMM Do YYYY, HH:mm:ss.SSS',
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this have to change for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the format that design recommended so it fits in the UI better. Spelling out the month wastes a lot of space

@elasticmachine
Copy link
Contributor

💔 Build Failed

@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 Feb 6, 2019

@lukasolson I had to revert the moving of fetch emit event from outside of are times different check. For a reason I have not been able to track down, that change broke dashboard cancel edit and reverting to old state (specifically the case tested by this functional test https://github.com/elastic/kibana/blob/master/test/functional/apps/dashboard/_view_edit.js#L121 - where you edit a dashboard, add a panel, cancel the changes, and the added panel should be removed).

I will create an issue for timefilter not emitting an event when refresh is checked

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese
Copy link
Contributor Author

nreese commented Feb 6, 2019

Created #30179 to track Clicking refresh button in Kibana top nav time picker does not cause refresh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Timepicker Timepicker Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants