[Lens] Add more in-editor Advanced documentation #86821
Conversation
| max-height: 500px; | ||
| max-width: 400px; | ||
| overflow-y: scroll; | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // Below 5 seconds the "humanize" call returns the "few seconds" sentence, which is not ok for ms | ||
| // This special config rewrite makes it sure to have precision also for sub-seconds durations | ||
| // ref: https://github.com/moment/moment/issues/348 | ||
| function wrapMomentPrecision<T>(callback: () => T): T { |
There was a problem hiding this comment.
This logic sets the display to "1 seconds" which should be "1 second" (not plural). Is that a moment issue?
There was a problem hiding this comment.
Ah, nice catch. I'll investigate
There was a problem hiding this comment.
Did an investigation on the topic.
TLDR; I would leave this glitch as for now, delaying the decision for when moment.js will be replaced in the codebase or if the localization of the humanize function would be eventually internally handled.
Longer explanation:
The issue is relative to moment.js as it is reproducible within the wrapMomentPrecision function with the following line:
console.log(moment.localeData().relativeTime(1, true, 'ss', false));moment.js has an internal RelativeTimeSpec configuration that leads to this result, and while it is possible to fix it with a configuration customization, it is rather hard to safely restore the default.
The fix is an override of the configuration with a function that replaces the template string in use to specifically handle the 1 second case. the problem is that it should be done for all supported locales (~134 in moment itself + custom one, not sure how many in Kibana - I've found one custom so far).
The default configuration restoration is hard as there's no (clean) direct way to access the raw RelativeTimeSpec object in use, but only access it's output. The thing is not impossible, but moment.js typings does not like it: there's an internal prop to be accessed which is not typed and not meant to be accessed.
A potential solution would rely on module augmentation:
// Need to augment the moment module to access a hidden config parameter
declare module 'moment' {
interface Locale {
_config: LocaleSpecification;
}
}
function wrapMomentPrecision<T>(callback: () => T): T {
...
const defaultLocaleConfig = moment.localeData()._config;
moment.updateLocale(moment.locale(), {
relativeTime: {
// what about other locales than "en" here?
ss: (n: number): string => n === 1 ? 'a second' : `${n} second`
},
});
// Execute the format/humanize call in the callback
const result = callback();
// restore all the default values now in moment to not break it
moment.updateLocale(moment.locale(), defaultLocaleConfig);
}It would be possible to propose upstream to give access to the LocaleSpecification data/configuration (the parent object of RelativeTimeSpec) but it is not guaranteed to be merged or accepted (moment.js is currently in maintenance mode).
Also it may requires to list and cover all potential locales in Kibana as well.
| const result = callback(); | ||
|
|
||
| // restore all the default values now in moment to not break it | ||
| units.forEach(({ unit }, i) => moment.relativeTimeThreshold(unit, defaultValues[i])); |
There was a problem hiding this comment.
If there's only one unit, why loop?
There was a problem hiding this comment.
It used to be multiple unit settings.
I've left the loop for now in case I need to tweak some other units: can refactor to single one if it's all good.
There was a problem hiding this comment.
Found some rounding errors, so added few more units to the list.
| ownFocus | ||
| isOpen={isPopoverOpen} | ||
| button={ | ||
| <EuiLink onClick={() => setIsPopoverOpen(!isPopoverOpen)}> |
There was a problem hiding this comment.
It wasn't obvious that this was clickable to me. How about making it primary color or adding an icon?
| advancedDescription: ( | ||
| <p> | ||
| {i18n.translate('xpack.lens.indexPattern.dateHistogram.autoAdvancedExplanation', { | ||
| defaultMessage: 'The specific interval adopted follow the logic in this table:', |
There was a problem hiding this comment.
| defaultMessage: 'The specific interval adopted follow the logic in this table:', | |
| defaultMessage: 'The specific interval follows this logic:', |
| { | ||
| field: 'bound', | ||
| name: i18n.translate('xpack.lens.indexPattern.dateHistogram.autoBoundHeader', { | ||
| defaultMessage: 'Target Interval measured', |
There was a problem hiding this comment.
| defaultMessage: 'Target Interval measured', | |
| defaultMessage: 'Target interval measured', |
| /> | ||
| } | ||
| closePopover={() => setIsPopoverOpen(false)} | ||
| anchorPosition="leftCenter" |
There was a problem hiding this comment.
The current position of the popover overlaps the label for the form. Would it be possible to change the anchorPosition, and also to make the popover button trigger from EuiFormRow instead of the ButtonIcon?
There was a problem hiding this comment.
Did a quick change to the EuiFormRow positioning:
It looks a bit confusing to me given the Create custom ranges link just below.
Probably putting it on the right hand side may looks better: my only concern is that if you pass to custom intervals and then get back the cursor will hover it (not a major concern):
In this last case it makes sense to move the anchorPosition to upRight.
There was a problem hiding this comment.
Pushed the second version in the PR.
|
@elasticmachine merge upstream |
dej611
added
release_note:enhancement
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
|
While testing the moment i18n I noticed the
The picture above should be |
|
@elasticmachine merge upstream |
| button={ | ||
| <HelpPopoverButton onClick={() => setIsPopoverOpen(!isPopoverOpen)}> | ||
| {i18n.translate('xpack.lens.indexPattern.movingAverage.helpText', { | ||
| defaultMessage: 'How does it work?', |
There was a problem hiding this comment.
I suggest that we use this format for the text throughout:
How it works
How moving average works
| defaultMessage='The size of the interval is chosen to be a "nice" value. It is possible for the chosen | ||
| interval to stay the same when changing the granularity slider if the "nice" interval is | ||
| the same. The minimum granularity is 1, and the maximum is | ||
| {setting}. To change the maximum granularity setting, go to Advanced settings.' |
There was a problem hiding this comment.
@MichaelMarcialis Can popovers have links? If so, it might be nice to link to Advanced Settings. Or, are they similar to tooltips, which don't allow links.
There was a problem hiding this comment.
Yes, popovers can include links.
Good question. In this case, I believe the pattern is that the |
| /> | ||
| </p> | ||
|
|
||
| <p> | ||
| <FormattedMessage | ||
| id="xpack.lens.indexPattern.movingAverage.longerExplanation" | ||
| defaultMessage="The Lens Moving Average uses a simple arithmetic mean of the window and applies a skip policy for gaps: | ||
| this means that for missing values the bucket is skipped and the calculation is performed on the next one." | ||
| defaultMessage="To calculate the moving average, **Lens** uses the mean of the window and applies a skip policy for gaps. |
There was a problem hiding this comment.
Markdown isn't supported here, so the bold doesn't show. There's a few places we need to update this.
| /> | ||
| </p> | ||
|
|
||
| <ul> |
There was a problem hiding this comment.
Also there is no moving average for the first position- we should probably show the missing values too.
| defaultMessage: `When there are missing values in an area or line chart these can be represented in different ways in **Lens**. Missing values are drawn using dotted | ||
| lines, and will not fill missing values at the left side of the chart.`, |
There was a problem hiding this comment.
I think this can be condensed more because the first sentence is pretty vague. How about "Gaps in the data are not shown by default, but can be represented as dotted lines with different modes."
|
@elasticmachine merge upstream |
|
@gchaps Changed the title pattern to be more subject specific. I'd say the length of the link is now pretty long, but I don't have a strong opinion here.
@wylieconlon removed the markdown bold from text. |
|
@dej611 I agree with you that the link text is too long. What I mean to say was:
|
|
@elasticmachine merge upstream |
|
@wylieconlon I think I did address your concern. In case we could resolve the remaining issue offline and move this PR forward. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Michael Marcialis <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Michael Marcialis <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Wylie Conlon <[email protected]> Co-authored-by: Michael Marcialis <[email protected]>
|
@dej611 This component looks really useful! We are working on updating ILM and are finding it difficult to explain "rollover" in a concise way. Some kind of in-product documentation would be really useful there. Is the idea that plugins develop their own solutions for now or is there a plan to have a shared component in Kibana? |
|
I am no aware of any particular Kibana-wide approach to in-product documentation like this. |
Summary
Fixes #81780
This PR is a first attempt to implement more advanced documentation in product using the popover component, as suggested in the ref issue.
In this PR the definition type interface has been also extended to support a
getHelpMessagefunction which will render a help text when available. This has been useful in the case of the date histogram definition.Implemented documentation:
How does interval granularity work?
Previous version
How does auto date histogram work?
Previous version
Checklist
Delete any items that are not applicable to this PR.