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
allow Localized with no children #230
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this will make it easier to use <Localized>
is these simple scenarios when using a string is enough. Thanks for the PR!
fluent-react/src/localized.js
Outdated
@@ -82,7 +82,8 @@ export default class Localized extends Component { | |||
render() { | |||
const { l10n } = this.context; | |||
const { id, attrs, children } = this.props; | |||
const elem = Children.only(children); | |||
const elem = typeof(children) !== "undefined" ? | |||
Children.only(children) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should still allow fallback strings in the JSX:
<Localized id="hello">
Hello, world!
</Localized>
In such cases, Localized
still receives a single child.
fluent-react/src/localized.js
Outdated
@@ -116,6 +117,10 @@ export default class Localized extends Component { | |||
} | |||
} | |||
|
|||
if (!elem) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given my comment above, I think it would be better to test !isValidElement(elem)
. You can also move this up a bit, above the logic which translates attributes which doesn't apply for strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that makes sense -- I've changed this to use just the message if children isn't a valid react element as suggested.
<Localized />, | ||
{ context: { l10n: new ReactLocalization([]) } } | ||
); | ||
assert.equal(wrapper.length, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please bring this test back and add a new one in localized_render_test.js
which tests <Localized>
with a string-typed child.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test is still there -- I've just changed it not to check for an exception as it doesn't throw one anymore.
I've added a few test cases for fallback string children (with/without message) and empty children (with/without message) as suggested.
…not have markup children
I've updated the PR with the requested changes. Sorry for the delay! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks for the updated PR and for your patience waiting for the review! I have just one more small request about not removing the elem
variable to make the code easier to understand for me. Otherwise, all looks good!
fluent-react/src/localized.js
Outdated
|
||
if (!l10n) { | ||
// Use the wrapped component as fallback. | ||
return elem; | ||
return children; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep the previous version with elem
, please. It makes it clearer that there should only be one child element. Same comment applies to other places in this PR.
I've (at last!) renamed children back to elem as requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @blushingpenguin!. LGTM.
Note to self: we'll likely need to use the React.Fragment
API in #186 in cases when the source content is a simple string and the localizer wants to insert text-level elements such as <em>
.
This allows which returns a string. This saves having to wrap all text content in an element, which is sometimes undesirable. (I think that returning strings from a components render function was added in React 16).