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

Split Localized into LocalizedElement, LocalizedText #502

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

stasm
Copy link
Contributor

@stasm stasm commented Jul 8, 2020

Fix #498.

@stasm stasm force-pushed the localized-split branch 2 times, most recently from ef51838 to 1803c30 Compare July 10, 2020 16:21
@stasm
Copy link
Contributor Author

stasm commented Jul 10, 2020

The PR is not ready for review yet. I need to write tests for the new LocalizedElement and LocalizedText components and I think writing them will require some refactoring of the current tests, unfortunately. I'm also not 100% sure I got all the types right.

Summary: in React, props.children can be any of the following:

  1. null or undefined,
  2. a string, number or a boolean,
  3. a React element, including a React fragment,
  4. an array of React elements.
  • LocalizedElement supports (3) above.
  • LocalizedText supports (1) and the string type from (2).

I'm not sure how much validation these new components should perform. For example, should LocalizedText throw if passed an element child? Or multiple element children? Or, should it replace them with the translation text?

@elisehein What do you think?

@elisehein
Copy link

It would make sense to me if correct use of the two components is enforced. Thinking back to your comment here even if it's possible for LocalizedText to handle simple element children, it would break down as soon as you try to use elems, in which case it would be nice to let the user know what the problem is. Perhaps not throw an error, but log a warning so that it still works for the simple use cases?

As an aside, will LocalizedElement support 4. an array of React elements.?

@stasm
Copy link
Contributor Author

stasm commented Jul 13, 2020

It would make sense to me if correct use of the two components is enforced. Thinking back to your comment here even if it's possible for LocalizedText to handle simple element children, it would break down as soon as you try to use elems, in which case it would be nice to let the user know what the problem is. Perhaps not throw an error, but log a warning so that it still works for the simple use cases?

Good point about validation. It's good to be explicit about the expected input, both from the PoV of the users of @fluent/react and its maintainers. I actually think it's beneficial to throw in case of invalid input. I implemented it in 75e1c76.

As an aside, will LocalizedElement support 4. an array of React elements.?

I don't think it should. Since one LocalizedElement corresponds to one Fluent message, I'm not sure what the correct behavior should be if there are more than one child. Am I missing something?

@elisehein
Copy link

elisehein commented Jul 15, 2020

I don't think it should. Since one LocalizedElement corresponds to one Fluent message, I'm not sure what the correct behavior should be if there are more than one child. Am I missing something?

No, not at all. I'm not sure either, it's a bit of a weird gray area imo... But was just wondering because you wrote it would support React fragments, which could also be a list of multiple children.

What would be the correct Fluent way of translating, say, this bit in your comment above? (genuinely asking, not suggesting)

Summary: in React, props.children can be any of the following:

1. null or undefined,
2. a string, number or a boolean,
3. a React element, including a React fragment,
4. an array of React elements.

It could be seen as a separate introductory phrase followed by four separate short phrases, but I guess it could also be seen as a single sentence that should be translated whole, in one <LocalizedElement>. In the latter case, I'd expect to be able to nest <p> and <ol> as the multiple sibling children of one <LocalizedElement>.

@stasm
Copy link
Contributor Author

stasm commented Jul 16, 2020

No, not at all. I'm not sure either, it's a bit of a weird gray area imo... But was just wondering because you wrote it would support React fragments, which could also be a list of multiple children.

As far as fragments are concerned, I'd like to support them explicitly via <LocalizedElement><>...</></LocalizedElement>. This has the benefit of still being only a single direct child.

The support for arrays and implicit fragments is something that we could consider in the future, and I think the current API could be extended in a backwards-compatible manner.

It could be seen as a separate introductory phrase followed by four separate short phrases, but I guess it could also be seen as a single sentence that should be translated whole, in one <LocalizedElement>. In the latter case, I'd expect to be able to nest <p> and <ol> as the multiple sibling children of one <LocalizedElement>.

That's a great question! Fluent's primary use-case is localizing the UI, and I'd argue that the example you cited is more content than UI. We've had some ideas about how to bridge the gap between these two different use-cases and how to support larger pieces of text in a single Fluent message. Today, the overlay mechanism (in both @fluent/react and @fluent/dom) only supports flat markup, i.e. no nested elements can appear in the source nor in the translation. There have been experiments centered around lifting this limitation (e.g. zbraniecki/fluent-domoverlays-js#6) but I don't expect us to revisit them in the near future, unfortunately. For now, dividing your example into 5 messages would be the easiest work-around.

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.

@fluent/react – does/should <Localized> work with a text node as a direct child?
2 participants