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

[Metrics UI] Alert Preview displays "There were 1 instance" because of convoluted i18n #89807

Closed
Zacqary opened this issue Jan 29, 2021 · 6 comments · Fixed by #90070
Closed
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services

Comments

@Zacqary
Copy link
Contributor

Zacqary commented Jan 29, 2021

Screen Shot 2021-01-29 at 5 17 02 PM

This isn't such an easy bug to fix because of the complexity of the i18n strings in the Alert Preview: https://github.com/elastic/kibana/blob/master/x-pack/plugins/infra/public/alerting/common/components/alert_preview.tsx#L162-L204

We need to add a was/were differentiator to the plural, but if we put it inside the currently existing instance plural, it'd format like:

Screen Shot 2021-01-29 at 5 18 50 PM

because we need to switch to an embedded <FormattedString> in order to make it bold.

Not sure what the best practice is regarding i18n and embedded <strong> elements.

@Zacqary Zacqary added bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Jan 29, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@Zacqary
Copy link
Contributor Author

Zacqary commented Jan 29, 2021

Could use some help from @elastic/kibana-localization on this.

@Zacqary
Copy link
Contributor Author

Zacqary commented Feb 1, 2021

Not sure what the best practice is regarding i18n and embedded <strong> elements.

Turns out that switching to embedded <FormattedString> is, in fact, the best practice. If I remember correctly it'd be difficult to move the was/were into yet another variable, though, because of some quirks with the way the i18n plural system works.

@Bamieh
Copy link
Member

Bamieh commented Feb 2, 2021

Syncing our discussion on slack:

We have a similar usecase documented in the guide. please have a look here https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/GUIDELINE.md

<FormattedMessage
  id="kbn.management.createIndexPattern.step.status.successLabel.successDetail"
  defaultMessage="{strongSuccess} Your index pattern matches {strongIndices}."
  values={{
    strongSuccess: (
      <strong>
        <FormattedMessage
          id="kbn.management.createIndexPattern.step.status.successLabel.strongSuccessLabel"
          defaultMessage="Success!"
        />
      </strong>),
    strongIndices: (
      <strong>
        <FormattedMessage
          id="kbn.management.createIndexPattern.step.status.successLabel.strongIndicesLabel"
          defaultMessage="{indicesLength, plural, one {# index} other {# indices}}"
          values={{ indicesLength: exactMatchedIndices.length }}
        />
      </strong>)
  }}
/>

Note that there are better alternatives out there but have to upgrade our intl libraries first to enable them.

@Zacqary
Copy link
Contributor Author

Zacqary commented Feb 2, 2021

@Bamieh The challenge is that the plurality needs to affect both bolded and un-bolded parts of the string. I think we could do something like this:

    <FormattedMessage
      id="xpack.infra.metrics.alertFlyout.alertPreviewResultInstances"
      defaultMessage="There {wereWas} {firedTimes} that satisfied the conditions of this alert"
      values={{
        wereWas: (
          <FormattedMessage
            id="xpack.infra.metrics.alertFlyout.wereWas"
            defaultMessage="{plurality, plural, one {was} other {were}}"
            values={{
              plurality: previewResult.resultTotals.fired,
            }}
          />
        ),
        firedTimes: (
          <strong>
            <FormattedMessage
              id="xpack.infra.metrics.alertFlyout.firedTimes"
              defaultMessage="{fired, plural, one {# instance} other {# instances}}"
              values={{
                fired: previewResult.resultTotals.fired,
              }}
            />
          </strong>
        ),
      }}
    />

But I believe this fails the i18n check because it thinks that there's unused values.

EDIT: I tried the i18n tests again after writing this comment and they're no longer failing? I'll try pushing this to the CI and see if it breaks anything.

@Bamieh
Copy link
Member

Bamieh commented Feb 3, 2021

@Zacqary The easy way out of this is to simply drop of the phrase "There was/were" from the message:

defaultMessage="{firedTimes} satisfied the conditions of this alert"

This would be my recommendation since it is more succent and avoids all the above complications.

On the other hand if you folks insist on having this exact message I recommend going with this approach:

<FormattedMessage
  id="xpack.infra.metrics.alertFlyout.alertPreviewResultInstances"
  defaultMessage="There {fired, plural, one {was} other {were}} {instancesHtml} that satisfied the conditions of this alert"
  values={{
    fired: previewResult.resultTotals.fired,
    instancesHtml: (
      <strong>
        <FormattedMessage
          id="xpack.infra.metrics.alertFlyout.numberOfInstances"
          defaultMessage="{fired, plural, one {# instance} other {# instances}}"
          values={{
            fired: previewResult.resultTotals.fired,
          }}
        />
      </strong>
    ),
  }}
/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants