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

Update translated example for Character Count in the review app #2934

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

romaricpascal
Copy link
Member

@romaricpascal romaricpascal commented Oct 20, 2022

  • Updates the character counting component with translations
  • Adds a word counting component, to illustrate variations between "one", "two" and "other"
  • Adds a warning notification banner to tell not to use our translations (which I'd be very happy for the wording to be tweaked if necessary @claireashworth).

A shortcut to the resulting page: https://govuk-frontend-pr-2934.herokuapp.com/examples/translated

This should be the last step to close #2701 🎉

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 20, 2022 16:14 Inactive
@romaricpascal romaricpascal requested a review from a team October 20, 2022 16:19
@@ -36,6 +36,21 @@
{{ govukBackLink({
"href": "/"
}) }}

{% call govukNotificationBanner({
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we should go with the Full page examples approach instead?
https://github.com/alphagov/govuk-frontend/blob/main/app/views/full-page-examples/index.njk#L17

Don't think Notification Banner fits the purpose

Use a notification banner to tell the user about something they need to know about, but that’s not directly related to the page content.

If the information is directly relevant to the thing the user is doing on that page, put the information in the main page content instead. Use inset text or warning text if it needs to stand out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something plain and simple? @claireashworth

<h1 class="govuk-heading-xl">
  Translated examples
</h1>

<p class="govuk-body-l">These examples are designed to test how patterns and components can be translated.</p>

<p class="govuk-body">Please do not use these translations directly in your application.</p>

Image showing the suggested text

Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, I would swap the second 2 lines to make extra sure the user sees the warning.
And how about this:

'These examples only illustrate how our components display translated text.'

@romaricpascal romaricpascal force-pushed the character-count-translated-example branch from b6af0e5 to 7f7a50d Compare October 24, 2022 10:26
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 24, 2022 10:26 Inactive
@romaricpascal
Copy link
Member Author

Good spot on the misuse of the component, @colinrotherham. I've amended the way the warning is displayed (and used a warning text to make sure the warning stands out @claireashworth).

Screenshot 2022-10-24 at 11 30 57

@romaricpascal romaricpascal force-pushed the character-count-translated-example branch from 7f7a50d to 82eeeb6 Compare October 24, 2022 10:33
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 24, 2022 10:33 Inactive
@romaricpascal romaricpascal force-pushed the character-count-translated-example branch from 82eeeb6 to f02f086 Compare October 24, 2022 10:40
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 24, 2022 10:40 Inactive
}) }}

<p class="govuk-body">
These examples only illustrate how our components display translated text.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @romaricpascal

I can see pushes going up with different text changes being tried out

Think we need to apply the "two thirds" column layout, maybe keep govuk-body-l on the opening text, and go with the --xl break like some of the other pages do (compare the two visually?) but I'll take a 2nd opinion

Try this:

<div class="govuk-grid-row">
  <div class="govuk-grid-column-two-thirds">
    <h1 class="govuk-heading-xl">Translated examples</h1>

    <p class="govuk-body-l">
      These examples only illustrate how our components display translated text.
      We do not guarantee that they are accurate.
    </p>

    {{ govukWarningText({
      text: "Please do not use these translations directly in your application.",
      iconFallbackText: "Warning"
    }) }}

    <hr class="govuk-section-break govuk-section-break--xl">
  </div>
</div>

☝️ @claireashworth suggested putting the warning first to make sure it's seen, but not sure if that's necessary now we're using govukWarningText() so I've swapped back

Screenshot showing HTML example above

@romaricpascal romaricpascal force-pushed the character-count-translated-example branch from f02f086 to 78012a5 Compare October 24, 2022 15:41
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 24, 2022 15:41 Inactive
@romaricpascal romaricpascal force-pushed the character-count-translated-example branch from 78012a5 to 5d7f086 Compare October 24, 2022 15:46
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2934 October 24, 2022 15:46 Inactive
@romaricpascal
Copy link
Member Author

Updated with Colin's latest proposal for the style of the warning, which had a much nicer hierarchy 🙌🏻 .

At the end of the day, that warning only appears to people that would stumble upon one our review apps, so I'm not sure how much back-and-forth we should have about it given its very limited audience, as long as it's clear enough 😄

@romaricpascal romaricpascal merged commit 4b660ef into main Oct 25, 2022
@romaricpascal romaricpascal deleted the character-count-translated-example branch October 25, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to pass translation strings into character count component via data-attributes
4 participants