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 all 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
20 changes: 16 additions & 4 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";
import VOID_ELEMENTS from "../vendor/voidElementTags";
Expand Down Expand Up @@ -79,8 +79,13 @@ export default class Localized extends Component {

render() {
const { l10n, parseMarkup } = this.context;
const { id, attrs, children } = this.props;
const elem = Children.only(children);
const { id, attrs, children: elem } = this.props;

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

if (!l10n) {
// Use the wrapped component as fallback.
Expand All @@ -101,6 +106,13 @@ export default class Localized extends Component {
attrs: messageAttrs
} = l10n.formatCompound(bundle, 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(elem)) {
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 Down Expand Up @@ -175,5 +187,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 FluentBundle();
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 FluentBundle();
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 FluentBundle();
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 FluentBundle();
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