-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Formatters] Add new currency and short number formatter, replacing default number and percent #53972
Conversation
This comment has been minimized.
This comment has been minimized.
...egacy/ui/public/field_editor/components/field_format_editor/editors/default_number/number.js
Outdated
Show resolved
Hide resolved
@wylieconlon I love where this is going. I especially like the way the formatters explain where the defaults are coming from. I think we maybe need to think about this as part of the choose locale in browser work @Bamieh has some prototype work for. Would love to sync up on this some. |
@wylieconlon I've tried pulling the code locally to play around with it but the bootstrap is failing: Looking at the code in the PR the general approach looks good to me. I am not sure what set of locales we want to provide for number formatting. For currencies, the top 35 currencies sound reasonable. |
@Bamieh Bootstrap fails due to some type checking issues, but the code still starts. |
@@ -18,3 +18,4 @@ | |||
*/ | |||
|
|||
export { FieldFormatEditor } from './field_format_editor'; | |||
export { editors } from './get_editors'; |
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.
This export is one way to access the same editors in Lens and Management. This is no longer the desired UX for Lens, so I will probably remove this.
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 w/ tweaks.
src/legacy/core_plugins/vis_type_timeseries/public/components/lib/__tests__/tick_formatter.js
Outdated
Show resolved
Hide resolved
src/plugins/data/common/field_formats/converters/intl_number_format.ts
Outdated
Show resolved
Hide resolved
abstract getArguments: () => Record<string, unknown>; | ||
|
||
protected getConvertedValue(val: any): string { | ||
if (val === -Infinity) return '-∞'; |
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.
Should these be internationalized?
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.
They aren't currently for the regular number formatter, which this is based on.
if (isNaN(val)) return ''; | ||
|
||
const defaultLocale = this.getConfig && this.getConfig('format:defaultLocale'); | ||
let locales = [defaultLocale, 'en']; |
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.
Nit: This and the if block would be tidier like this:
const locales = defaultLocale === 'detect'
? navigator.languages.concat(['en'])
: [navigator.language].concat([defaultLocale, 'en']);
I had to stare at this a bit longer than necessary as it is.
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.
Even more nit:
const locales = defaultLocale === 'detect'
? [...navigator.languages, 'en']
: [navigator.language, defaultLocale, 'en'];
pattern: this.getConfig!('format:percent:defaultPattern'), | ||
fractional: true, | ||
minDecimals: 0, | ||
maxDecimals: 3, |
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.
I wonder if 3 is the right default for percentage? I almost think maybe 0 would be the right default, but maybe not.
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.
I think I will keep it at 3 because it's the minimum possible change- 3 was the previous default.
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.
Looks good so far (want to wait for final tests/migration before approving). Left a couple of comments. Since https://github.com/elastic/kibana/pull/54240/files has been merged, I think we need to incorporate that flag also in the new field formatters in this PR.
import { UrlFormatEditor } from './editors/url/url'; | ||
|
||
export const editors = [ | ||
BytesFormatEditor, |
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.
Unfortunately the order of this array is currently also used for ordering the select in the UI. Since it was ordered alphabetically so far, I think we should keep this array alphabetically (by their English names) sorted (until we might in a later step properly sort them in some other place via their names).
src/legacy/ui/public/field_editor/components/field_format_editor/editors/currency/currency.js
Outdated
Show resolved
Hide resolved
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Updates:
|
Reporting team has another top focus right now, which is our new platform migration. Our ongoing changes will likely cause conflicts in this PR. We would like to better understand what kind of formatting would be left if number formatting goes away for CSV export, and if there is any, would the remainder be useful to keep? We have a discuss issue relevant to this: #55484 If the proposal is accepted, then your point about this wouldn't be a blocker. But usually we are very careful not to ship breaking changes in a minor release. If not done carefully it just makes it look like we have shipped a bug. |
@wylieconlon Can you explain what the breaking change is and why #34996 would solve it? Would it be possible to make these changes without a migration by "migrating" index-patterns inside the data plugin before exposing them to other plugins? I'm assuming plugins don't directly call the SavedObjectsClient to retrieve index patterns so if we have a share API for reading them that all plugins use, we could apply the migration there and at the same time have access to the config object. |
@rudolf Even if your assumption that there is only one way to read index patterns were accurate (it isn't, as far as I know), it would not solve the problem. The reason the migration needs the configuration is that the logic needs to be as follows:
|
@wylieconlon Now that Github allows it, should we set this PR back to draft? Just been hanging out in review queues for a long time now. |
Despite support for the new formatting options in Node 14, I am going to close this PR as I don't expect to make the changes here. |
Release note
Adds new number formatters for currency and short numbers (thousands, millions), and allow # of decimals to be customized on most number formatters. The new formatters have a wider range of locales and currencies supported, and can target your browser's locale for display.
This change adds the following advanced settings:
format:defaultLocale
format:default_number:maxDecimals
format:currency:defaultCurrency
format:currency:maxDecimals
CSV export no longer formats numbers because node.js does not support localization in the version used by Kibana.
Deprecation note
Numeral.js patterns are no longer supported on the
bytes
andpercent
formatter. The following settings have been deprecated, and no longer have any affect:format:bytes:defaultPattern
format:currency:defaultPattern
format:percent:defaultPattern
While these settings are deprecated, there remain custom formatters in TSVB, Canvas, and SIEM that map to the deprecated settings.
Summary
Closes #39211 and closes #25993
The user-facing change here is that there are new number formatters that are more flexible than our previous ones which required knowledge of the numeral.js syntax. The higher level of flexibility is a pre-requisite for our usage of formatters in Lens. The new formatters are using the standard
Intl.NumberFormat
API, which is supported by all browsers we support.Summary of changes:
de-DE
for German in Germany will get the following languages:['de-DE', 'de', 'en']
which will be used in order for formatting.Numeral.js does not use the same technique because of its limited support, and it's one reason it's minimized in this change.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers