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

allow Localized with no children #230

Merged
merged 4 commits into from
Sep 24, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 22 additions & 10 deletions fluent-react/src/localized.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isValidElement, cloneElement, Component, Children } from "react";
import { isValidElement, cloneElement, Component } from "react";
import PropTypes from "prop-types";

import { isReactLocalization } from "./localization";
Expand Down Expand Up @@ -82,18 +82,23 @@ export default class Localized extends Component {
render() {
const { l10n } = this.context;
const { id, attrs, children } = this.props;
const elem = Children.only(children);

// Validate that children isn't an array
if (Array.isArray(children)) {
throw new Error("<Localized/> expected to receive a single " +
"React node child");
}

if (!l10n) {
// Use the wrapped component as fallback.
return elem;
return children;
Copy link
Contributor

@stasm stasm Aug 9, 2018

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.

}

const mcx = l10n.getMessageContext(id);

if (mcx === null) {
// Use the wrapped component as fallback.
return elem;
return children;
}

const msg = mcx.getMessage(id);
Expand All @@ -103,6 +108,13 @@ export default class Localized extends Component {
attrs: messageAttrs
} = l10n.formatCompound(mcx, msg, args);

// Check if the fallback is a valid element -- if not then it's not
// markup (e.g. nothing or a fallback string) so just use the
// formatted message value
if (!isValidElement(children)) {
return messageValue;
}

// The default is to forbid all message attributes. If the attrs prop exists
// on the Localized instance, only set message attributes which have been
// explicitly allowed by the developer.
Expand All @@ -120,21 +132,21 @@ export default class Localized extends Component {
// message value and do not pass it to cloneElement in order to avoid the
// "void element tags must neither have `children` nor use
// `dangerouslySetInnerHTML`" error.
if (elem.type in VOID_ELEMENTS) {
return cloneElement(elem, localizedProps);
if (children.type in VOID_ELEMENTS) {
return cloneElement(children, localizedProps);
}

// If the message has a null value, we're only interested in its attributes.
// Do not pass the null value to cloneElement as it would nuke all children
// of the wrapped component.
if (messageValue === null) {
return cloneElement(elem, localizedProps);
return cloneElement(children, localizedProps);
}

// If the message value doesn't contain any markup nor any HTML entities,
// insert it as the only child of the wrapped component.
if (!reMarkup.test(messageValue)) {
return cloneElement(elem, localizedProps, messageValue);
return cloneElement(children, localizedProps, messageValue);
}

// If the message contains markup, parse it and try to match the children
Expand Down Expand Up @@ -167,7 +179,7 @@ export default class Localized extends Component {
return cloneElement(sourceChild, null, childNode.textContent);
});

return cloneElement(elem, localizedProps, ...translatedChildren);
return cloneElement(children, localizedProps, ...translatedChildren);
}
}

Expand All @@ -176,5 +188,5 @@ Localized.contextTypes = {
};

Localized.propTypes = {
children: PropTypes.element.isRequired,
children: PropTypes.node
};
61 changes: 61 additions & 0 deletions fluent-react/test/localized_render_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,4 +262,65 @@ foo = { $arg }
const { args } = format.getCall(0);
assert.deepEqual(args[1], { arg: 'ARG' });
});

test('render with a string fallback and no message returns the fallback',
function() {
const mcx = new MessageContext();
const l10n = new ReactLocalization([mcx]);

const wrapper = shallow(
<Localized id="foo">
String fallback
</Localized>,
{ context: { l10n } }
);

assert.equal(wrapper.text(), 'String fallback');
});

test('render with a string fallback returns the message', function() {
const mcx = new MessageContext();
const l10n = new ReactLocalization([mcx]);
mcx.addMessages(`
foo = Test message
`)

const wrapper = shallow(
<Localized id="foo">
String fallback
</Localized>,
{ context: { l10n } }
);

assert.equal(wrapper.text(), 'Test message');
});

test('render without a fallback returns the message', function() {
const mcx = new MessageContext();
const l10n = new ReactLocalization([mcx]);

mcx.addMessages(`
foo = Message
`)

const wrapper = shallow(
<Localized id="foo" />,
{ context: { l10n } }
);

assert.equal(wrapper.text(), 'Message');
});

test('render without a fallback and no message returns nothing',
function() {
const mcx = new MessageContext();
const l10n = new ReactLocalization([mcx]);

const wrapper = shallow(
<Localized id="foo" />,
{ context: { l10n } }
);

assert.equal(wrapper.text(), '');
});
});
14 changes: 6 additions & 8 deletions fluent-react/test/localized_valid_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,11 @@ suite('Localized - validation', function() {
});

test('without a child', function() {
function render() {
shallow(
<Localized />,
{ context: { l10n: new ReactLocalization([]) } }
);
}
assert.throws(render, /a single React element child/);
const wrapper = shallow(
<Localized />,
{ context: { l10n: new ReactLocalization([]) } }
);
assert.equal(wrapper.length, 1);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

});

test('with multiple children', function() {
Expand All @@ -64,7 +62,7 @@ suite('Localized - validation', function() {
{ context: { l10n: new ReactLocalization([]) } }
);
}
assert.throws(render, /a single React element child/);
assert.throws(render, /a single React node child/);
});

test('without id', function() {
Expand Down