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

Add advanced setting to control quick ranges #15975

Merged
merged 4 commits into from
Jan 12, 2018

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Jan 10, 2018

Closes #2758.

This PR creates a new advanced setting, timepicker:quickRanges which controls the list of ranges that show up in the "Quick" section of the time picker.

The default value for this is similar to what used to show up, minus the ranges that used to show up in the second column (e.g. "Yesterday", "Previous week", "Previous month", etc.). The impetus for removing this column is that it probably wasn't used very often and the user can easily use the forward/backward arrows next to the time picker to access the majority of these. Also, it frees up more space in the "Quick" section with the idea that this space can be used to show ranges from your history.

@lukasolson lukasolson self-assigned this Jan 10, 2018
@lukasolson lukasolson requested review from nreese and Bargs January 10, 2018 21:07
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This is great

{ from: 'now-5y', to: 'now', display: 'Last 5 years', section: 2 },

], null, 2),
description: 'The list of ranges to show in the Quick section of the time picker.'
Copy link
Contributor

@nreese nreese Jan 10, 2018

Choose a reason for hiding this comment

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

Is there any documentation that you could link too? It would be helpful to know what proper values can be used.

@@ -277,6 +277,37 @@ export function getUiSettingDefaults() {
}`,
description: 'The timefilter\'s default refresh interval'
},
'timepicker:quickRanges': {
Copy link
Contributor

@nreese nreese Jan 10, 2018

Choose a reason for hiding this comment

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

Is there a way to add a verify method to ensure the time ranges are valid? It would be better to get an error message and not update the quickRanges on the advanced setting page, then to let users save invalid time ranges and get a cryptic error when selecting the invalid range via the timepicker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there's any sort of verification currently (beyond verifying that the value actually is JSON). I could probably add a custom directive and handling for this but it might add more complexity than it's worth... Plus, there are other JSON values in the advanced settings that we don't have any sort of verification either. What if I just added some notes to the docs about how to configure the value and linked to it in the description?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Love it. Looks like one use of quick_ranges will need to be updated in x-pack as well.

LGTM

@@ -1,5 +1,6 @@
import moment from 'moment-timezone';
import numeralLanguages from '@elastic/numeral/languages';
// import { documentationLinks } from '../../ui/public/documentation_links';
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray comment?

return {
restrict: 'E',
scope: {
from: '=',
to: '='
},
link: function ($scope, $elem) {
const quickRanges = config.get('timepicker:quickRanges');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a check here to make sure users don't add a crazy number of sections?

Copy link
Member Author

Choose a reason for hiding this comment

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

Meh, we could, but I don't envision that being a super common problem and it's easily fixed.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm

@lukasolson lukasolson merged commit e6b65fc into elastic:master Jan 12, 2018
lukasolson added a commit to lukasolson/kibana that referenced this pull request Jan 12, 2018
* Add advanced setting to control quick ranges

* Fix test

* Add docs for quick ranges

* Remove comment
@lukasolson
Copy link
Member Author

6.x (6.2.0): 9578180

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