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

[I18n] Support interpreting individual i18n-values as html or text-only #26274

Conversation

LeanidShutau
Copy link
Contributor

@LeanidShutau LeanidShutau commented Nov 27, 2018

Resolves #26173

@LeanidShutau LeanidShutau added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Project:i18n labels Nov 27, 2018
@LeanidShutau LeanidShutau self-assigned this Nov 27, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@spalger
Copy link
Contributor

spalger commented Nov 27, 2018

Out of curiosity, why did we decide against flagging html/text per-variable? Saying all variables are html or not seems like something that would be easy to miss if someone comes in down the road and adds another variable.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@azasypkin
Copy link
Member

Based on our recent discussions here is the PoC that seems to work as we expect. As it's perf critical code I tried to be as "lazy" in costly operations as possible.

import { IDirective, IRootElementService, IScope } from 'angular';

import { I18nServiceType } from './provider';

interface I18nScope extends IScope {
  values?: Record<string, any>;
  defaultMessage: string;
  id: string;
}

const HTML_KEY_PREFIX = 'html_';
const PLACEHOLDER_SEPARATOR = '@I18N@';

export function i18nDirective(
  i18n: I18nServiceType,
  $sanitize: (html: string) => string
): IDirective<I18nScope> {
  return {
    restrict: 'A',
    scope: {
      id: '@i18nId',
      defaultMessage: '@i18nDefaultMessage',
      values: '<?i18nValues',
    },
    link($scope, $element) {
      if ($scope.values) {
        $scope.$watchCollection('values', () => setContent($element, $scope, $sanitize, i18n));
      } else {
        setContent($element, $scope, $sanitize, i18n);
      }
    },
  };
}

function setContent(
  $element: IRootElementService,
  $scope: I18nScope,
  $sanitize: (html: string) => string,
  i18n: I18nServiceType
) {
  const originalValues = $scope.values;
  const valuesWithPlaceholders = {} as Record<string, any>;
  let hasValuesWithPlaceholders = false;

  // If we have values with keys starts with HTML_KEY_PREFIX we should replace
  // them with special placeholders that later one will be inserted as HTML
  // into the DOM, the rest of the content will be treated as text. We don't
  // sanitize values at this stage as some of the values can be excluded from
  // the translated string (e.g. not used by ICU conditional statements).
  if (originalValues) {
    for (const [key, value] of Object.entries(originalValues)) {
      if (key.startsWith(HTML_KEY_PREFIX)) {
        valuesWithPlaceholders[
          key.slice(HTML_KEY_PREFIX.length)
        ] = `${PLACEHOLDER_SEPARATOR}${key}${PLACEHOLDER_SEPARATOR}`;

        hasValuesWithPlaceholders = true;
      } else {
        valuesWithPlaceholders[key] = value;
      }
    }
  }

  const label = i18n($scope.id, {
    values: valuesWithPlaceholders,
    defaultMessage: $scope.defaultMessage,
  });

  // If there are no placeholders to replace treat everything as text, otherwise
  // insert label piece by piece replacing every placeholder with corresponding
  // sanitized HTML content.
  if (!hasValuesWithPlaceholders) {
    $element.text(label);
  } else {
    $element.empty();
    for (const contentOrPlaceholder of label.split(PLACEHOLDER_SEPARATOR)) {
      if (!contentOrPlaceholder) {
        continue;
      }

      $element.append(
        originalValues!.hasOwnProperty(contentOrPlaceholder)
          ? $sanitize(originalValues![contentOrPlaceholder])
          : document.createTextNode(contentOrPlaceholder)
      );
    }
  }
}

@LeanidShutau would you mind proof reading this and take it as a base for your PR if you think it works as we expect?

@LeanidShutau LeanidShutau force-pushed the feature/i18n-values-html-text-differentiation branch from 4641dbb to aed376a Compare November 28, 2018 11:03
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of nits! Also would you mind adding this html_ thing into i18n Angular docs/guideline?

Please, wait for feedback from @spalger before merging this as well. Thanks!

packages/kbn-i18n/src/angular/directive.ts Outdated Show resolved Hide resolved
@azasypkin azasypkin requested a review from spalger November 28, 2018 15:12
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM

@spalger spalger changed the title [I18n] Add attribute for interpreting i18n-values as html or text-only [I18n] Support interpreting individual i18n-values as html or text-only Nov 28, 2018
@spalger
Copy link
Contributor

spalger commented Nov 28, 2018

Is there a documentation update we should make along with this?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

LGTM

@LeanidShutau LeanidShutau removed the request for review from pavel06081991 November 30, 2018 11:57
@LeanidShutau LeanidShutau merged commit c800f8d into elastic:master Nov 30, 2018
@LeanidShutau LeanidShutau deleted the feature/i18n-values-html-text-differentiation branch November 30, 2018 14:48
LeanidShutau added a commit to LeanidShutau/kibana that referenced this pull request Nov 30, 2018
…ly (elastic#26274)

* [I18n] Add attribute for interpreting i18n-values as html or text-only

* Switch over to html_ prefixed values solution

* Update readme
LeanidShutau added a commit that referenced this pull request Dec 3, 2018
…ly (#26274) (#26469)

* [I18n] Add attribute for interpreting i18n-values as html or text-only

* Switch over to html_ prefixed values solution

* Update readme
@LeanidShutau
Copy link
Contributor Author

6.x/6.6: f1f5f1c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Project:i18n Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v6.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants