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.

Sign up for GitHub

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

Jump to bottom

Translate Gauge and Goal #23882

Merged
merged 13 commits into from
Oct 19, 2018
Merged

Translate Gauge and Goal #23882

merged 13 commits into from
Oct 19, 2018

Conversation

tibmt
Copy link
Contributor

@tibmt tibmt commented Oct 5, 2018

Translation of Gauge and Goal visualization components

@tibmt tibmt self-assigned this Oct 5, 2018
@tibmt tibmt requested review from pavel06081991 and maryia-lapata and removed request for maryia-lapata October 5, 2018 15:18
@elasticmachine
Copy link
Contributor

💔 Build Failed

@pavel06081991
Copy link
Contributor

@tibmt add 'kbnVislibVisTypes' to .i18nrc.json for CI to pass CI validation

pavel06081991
pavel06081991 reviewed Oct 8, 2018
View reviewed changes
src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html Outdated
@@ -171,17 +205,25 @@
<div class="hintbox" ng-show="!editorState.params.gauge.colorsRange.length">
<p>
<i class="fa fa-danger text-danger"></i>
<strong>Required:</strong> You must specify at least one range.
<span
i18n-id="kbnVislibVisTypes.controls.gaugeOptions.requiredTitle"
Copy link
Contributor

Choose a reason for hiding this comment

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

requiredTitle => specifiedRangeNumberWarningMessage

src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html Outdated
<strong>Required:</strong> You must specify at least one range.
<span
i18n-id="kbnVislibVisTypes.controls.gaugeOptions.requiredTitle"
i18n-default-message="<strong>Required:</strong> You must specify at least one range."
Copy link
Contributor

Choose a reason for hiding this comment

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

use i18n-values for nested tags

src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html Outdated
Add Range
</div>
class="kuiButton kuiButton--primary kuiButton--fullWidth"
i18n-id="kbnVislibVisTypes.controls.gaugeOptions.addRangeButton"
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like it is title, not button, so addRangeButton => addRangeTitle

src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html Outdated
<div class="text text-center text-info">Note: colors can be changed in the legend</div>
<div
class="text text-center text-info"
i18n-id="kbnVislibVisTypes.controls.gaugeOptions.noteTitle"
Copy link
Contributor

Choose a reason for hiding this comment

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

noteTitle => howToChangeColorsDescription

src/core_plugins/kbn_vislib_vis_types/public/gauge.js Outdated
@@ -103,7 +104,7 @@ export default function GaugeVisType(Private) {
{
group: 'buckets',
name: 'group',
title: 'Split Group',
title: i18n('kbnVislibVisTypes.gauge.splitGroupTitle', { defaultMessage: 'Split Group' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

splitGroupTitle => groupTitle

src/core_plugins/kbn_vislib_vis_types/public/goal.js Outdated
@@ -98,7 +100,7 @@ export default function GoalVisType(Private) {
{
group: 'buckets',
name: 'group',
title: 'Split Group',
title: i18n('kbnVislibVisTypes.goal.splitGroupTitle', { defaultMessage: 'Split Group' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

splitGroupTitle => groupTitle

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

pavel06081991
pavel06081991 reviewed Oct 8, 2018
View reviewed changes
Copy link
Contributor

@pavel06081991 pavel06081991 left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tibmt
Copy link
Contributor Author

tibmt commented Oct 9, 2018

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

timroes
timroes suggested changes Oct 15, 2018
View reviewed changes
src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.html Outdated
for="showScale"
i18n-id="kbnVislibVisTypes.controls.gaugeOptions.fontSizeLabel"
i18n-default-message="Font Size ({fontSizeSpan}pt)"
i18n-values="{ fontSizeSpan: '<span ng-bind=\'editorState.params.gauge.style.fontSize\'></span>' }"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks wrong. This should rather just inject editorState.params.gauge.style.fontSize as a parameter and not that weird span ng-bind construct.

Also please rename the parameter name to fontSize, we should not keep element names (especially if they are not semantic elements at all) in the parameter name.

src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.js Outdated
@@ -107,6 +107,10 @@ module.directive('gaugeOptions', function () {
$scope.customColors = true;
});

$scope.editorState.requiredText = i18n('kbnVislibVisTypes.params.gauge.requiredText', {
defaultMessage: 'Requred:'
Copy link
Contributor

Choose a reason for hiding this comment

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

Required

src/core_plugins/kbn_vislib_vis_types/public/controls/gauge_options.js Outdated
@@ -107,6 +107,10 @@ module.directive('gaugeOptions', function () {
$scope.customColors = true;
});

$scope.editorState.requiredText = i18n('kbnVislibVisTypes.params.gauge.requiredText', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't set that value on the editorState, but use the $scope directly. A translated string is NOT part of the editor state.

src/core_plugins/kbn_vislib_vis_types/public/gauge.js Outdated
description: `Gauges indicate the status of a metric. Use it to show how a metric's value relates
to reference threshold values.`,
description: i18n('kbnVislibVisTypes.gauge.gaugeDescription', {
defaultMessage: 'Gauges indicate the status of a metric. Use it to show how a metric\'s value relatesto reference threshold values.'
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent the need of escaping the single quote, please just use a backtick string 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.

@timroes there will be error.

Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up that there's a missing space here between relatesto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukeelmers fix it, thanks

@tibmt
Copy link
Contributor Author

tibmt commented Oct 15, 2018

update Gauge(not Heatmap) component

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tibmt tibmt added the blocked label Oct 17, 2018
@tibmt tibmt removed the blocked label Oct 18, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tibmt
Copy link
Contributor Author

tibmt commented Oct 19, 2018

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tibmt
Copy link
Contributor Author

tibmt commented Oct 19, 2018

Now we cannot use backtick string or double quote. We will change it when it will be possible

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@maryia-lapata maryia-lapata merged commit cc75547 into elastic:master Oct 19, 2018
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Oct 19, 2018
@tibmt @maryia-lapata
Translate Gauge and Goal (elastic#23882)
720d09e 
* Translate Gauge and Goal

* fix some names, rewrite some code and add 'kbnVislibVisTypes' to '.i18nrc.json'

* add 'Required' translation

* fix 'requiredText' bug

* use \' instead of &quot;

* update heatmap component

* Update gauge.js

* Update gauge.js

* Update gauge.js
@maryia-lapata maryia-lapata mentioned this pull request Oct 19, 2018
Merged
maryia-lapata added a commit that referenced this pull request Oct 19, 2018
@maryia-lapata
Translate Gauge and Goal (#23882) (#24270)
0b2beed 
* Translate Gauge and Goal

* fix some names, rewrite some code and add 'kbnVislibVisTypes' to '.i18nrc.json'

* add 'Required' translation

* fix 'requiredText' bug

* use \' instead of &quot;

* update heatmap component

* Update gauge.js

* Update gauge.js

* Update gauge.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukeelmers lukeelmers lukeelmers left review comments

@pavel06081991 pavel06081991 pavel06081991 approved these changes

@timroes timroes timroes approved these changes

Assignees

@tibmt tibmt

Labels
backported Project:i18n v6.5.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tibmt @elasticmachine @pavel06081991 @timroes @lukeelmers @maryia-lapata