Skip to content

feat(widgets): add default translated texts#61

Merged
1 commit merged intoalphafrom
add-translations
May 20, 2020
Merged

feat(widgets): add default translated texts#61
1 commit merged intoalphafrom
add-translations

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented May 7, 2020

This adds default, translated texts for our widgets, where appropriate. So say, a placeholder for a filter input. Usually devs will leave those types of placeholders as is, so it makes sense for us to set a proper default.

Since we're not relying on the namespace being set correctly when the modules are parsed, we're delaying the call to i18n.t by allowing a function as prop for the translatable texts. A potential follow up: do we want to allow that for all our string/renderable text props, so that our users can do the same?

Currently I'm just changing things in widgets, so core components are not affected for the time being.

See also: https://dhis2.slack.com/archives/CBM8LNEQM/p1588767905388900 (translations)

Todo:

  • multiselect
  • singleselect
  • What do we do with props that accept a node (like the Select's empty prop)?
  • organisationunittree (doesn't seem like it needs translations?)
  • Add stories demonstrating texts
  • Add tests (verify that the translations work correctly)
  • Update jsdoc & prop-types
  • Verify the translations I've chosen (ask Joe)
  • Check if we can interpolate the filter noMatch text (unfortunately not with the current strategy)
  • Remove requiredif's everywhere
  • FileInput (verify translations, add demos and FileInputFieldWithList placeholder never shows #70, technically there are duplicate translations, but they do make the components a bit more independent maybe?)

Follow up:

  • allow func for all our string props?
  • transfer?
  • follow up fileinput (Cancel link (Cancel) & Loading state (Uploading))

@Mohammer5
Copy link
Copy Markdown
Contributor

The PR title is misleading. I was expecting all components in widgets to have been translated. Was that your original plan and you just wanted to confirm the approach before translating the remaining components?

@ghost
Copy link
Copy Markdown
Author

ghost commented May 7, 2020

The PR title is misleading. I was expecting all components in widgets to have been translated. Was that your original plan and you just wanted to confirm the approach before translating the remaining components?

It's still a draft, I'm still working on it. So yes, currently just seeing what the best approach would be.

@cypress
Copy link
Copy Markdown

cypress bot commented May 7, 2020



Test summary

438 0 0 0


Run details

Project ui
Status Passed
Commit c85342d
Started May 20, 2020 9:31 AM
Ended May 20, 2020 9:40 AM
Duration 08:31 💡
OS Linux Ubuntu Linux - 18.04
Browser Electron 80

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@ghost ghost mentioned this pull request May 7, 2020
Merged
15 tasks
@Mohammer5
Copy link
Copy Markdown
Contributor

I like the approach and wouldn't know a better way right now.
The only thing that bugs me a bit is that the default values' types don't match the prop types.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 7, 2020

I like the approach and wouldn't know a better way right now.
The only thing that bugs me a bit is that the default values' types don't match the prop types.

Yeah I haven't updated the jsdoc and the proptype for empty yet, but the rest should be good:

propTypes.oneOfType([propTypes.string, propTypes.func])

@ghost

This comment has been minimized.

@Mohammer5

This comment has been minimized.

@ghost ghost force-pushed the add-translations branch from 498102d to d763e21 Compare May 7, 2020 14:36
@ghost

This comment has been minimized.

@ghost ghost force-pushed the add-translations branch from 1d8b887 to 8eadf0d Compare May 11, 2020 14:54
@ghost
Copy link
Copy Markdown
Author

ghost commented May 12, 2020

Hey @cooper-joe, I was wondering what you think of the translations. These are the ones I've gone with for the Single- and MultiSelectField:

Screenshot 2020-05-12 at 13 46 08

Let me know if you'd prefer otherwise and I'll update them.

@ghost ghost marked this pull request as ready for review May 12, 2020 12:11
@ghost ghost self-requested a review as a code owner May 12, 2020 12:11
@cooper-joe
Copy link
Copy Markdown
Member

Hey @cooper-joe, I was wondering what you think of the translations.

Thanks for the ping! I'd like to request some minor changes:

empty -> "No data found"
filterPlaceholder -> "Type to filter options"
noMatchText -> "No options found"

noMatchText: An aside: I remember when this component was being built I requested a text here that would insert the filter term. For example if I filter by string dog and there are no matches the result would be "Nothing found for dog". Is this possible now we're adding translations to these components in ui-widgets? It's a handy way to give the user some feedback, answering any question ("Why is there nothing found...?") before it comes up.

@HendrikThePendric
Copy link
Copy Markdown
Contributor

HendrikThePendric commented May 12, 2020

Great work @ismay, glad to see these are all functions so we won't run into the issues we encountered last week in many repos.

The way we have organised the components has changed quite a bit lately so I'm not completely sure where we'd want to add this.... But according to our design system, the FileInput needs quite a few default texts too:

  • Button label (Upload a file / Upload files)
  • Remove link (Remove)
  • Empty list placeholder (No file(s) selected yet)

Could you add a ToDo item for that too?

In the design specs there are more default texts to be found:

  • Cancel link (Cancel)
  • Loading state (Uploading)

But I think these two are for later on, when we introduce a component that allows us to interact with the file-resources endpoint, i.e. with async functionality.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 13, 2020

noMatchText: An aside: I remember when this component was being built I requested a text here that would insert the filter term. For example if I filter by string dog and there are no matches the result would be "Nothing found for dog". Is this possible now we're adding translations to these components in ui-widgets? It's a handy way to give the user some feedback, answering any question ("Why is there nothing found...?") before it comes up.

@joe Yeah you're right, I remember that we had it and removed it because there were concerns about interpolation with RTL languages. I'm going to test this in the scheduler, see what the issue is exactly.

I've addressed these points btw:

  • empty -> "No data found"
  • filterPlaceholder -> "Type to filter options"
  • noMatchText -> "No options found"

@ghost
Copy link
Copy Markdown
Author

ghost commented May 13, 2020

Could you add a ToDo item for that too?

@HendrikThePendric N.p. 👍 I've added it to the top comment

@ghost
Copy link
Copy Markdown
Author

ghost commented May 13, 2020

noMatchText: An aside: I remember when this component was being built I requested a text here that would insert the filter term. For example if I filter by string dog and there are no matches the result would be "Nothing found for dog". Is this possible now we're adding translations to these components in ui-widgets? It's a handy way to give the user some feedback, answering any question ("Why is there nothing found...?") before it comes up.

So, this took a while, and I don't have a definitive answer, but it seems safe enough. See here: dhis2/notes#103. So we could use an interpolation and use Nothing found for {{ search term }}. Is that the translation you'd prefer @cooper-joe?

@cooper-joe
Copy link
Copy Markdown
Member

So we could use an interpolation and use Nothing found for {{ search term }}. Is that the translation you'd prefer @cooper-joe?

Great! Yes, let's use that for the default. I think it's best to include the search term inside single quotes to highlight it as user-content: Nothing found for 'malaria'.

@ghost
Copy link
Copy Markdown
Author

ghost commented May 13, 2020

So we could use an interpolation and use Nothing found for {{ search term }}. Is that the translation you'd prefer @cooper-joe?

Great! Yes, let's use that for the default. I think it's best to include the search term inside single quotes to highlight it as user-content: Nothing found for 'malaria'.

Actually, I was wrong, sorry. It's not possible to include the filter as that's not known at the point where we define the default. Unless we change our translation strategy, but that would be a major change. I'll leave it as is, if you're all right with that as well.

@cooper-joe
Copy link
Copy Markdown
Member

Actually, I was wrong, sorry. It's not possible to include the filter as that's not known at the point where we define the default. Unless we change our translation strategy, but that would be a major change. I'll leave it as is, if you're all right with that as well.

Ah ok. I see the problem. In that case, let's go for: No options found.

@ghost ghost added this to the v5.0.0 milestone May 14, 2020
@ghost
Copy link
Copy Markdown
Author

ghost commented May 14, 2020

@cooper-joe I've added three more default translations for the FileInputField and the FileInputFieldWithList:

  1. Remove (for the label to remove a file)
  2. Upload a file (for the button label)
  3. No file uploaded yet (as a placeholder)

Let me know what you think.

@cooper-joe
Copy link
Copy Markdown
Member

@cooper-joe I've added three more default translations for the FileInputField and the FileInputFieldWithList:

1. `Remove` (for the label to remove a file)

2. `Upload a file` (for the button label)

3. `No file uploaded yet` (as a placeholder)

Let me know what you think.

Those look good to me @ismay 👍

Copy link
Copy Markdown
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@ghost ghost force-pushed the add-translations branch from 6bae089 to c85342d Compare May 20, 2020 09:29
@ghost ghost merged commit 1f0192b into alpha May 20, 2020
@ghost ghost deleted the add-translations branch May 20, 2020 09:41
@dhis2-bot
Copy link
Copy Markdown
Contributor

@dhis2-bot
Copy link
Copy Markdown
Contributor

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants