DOM overlays for fluent-react #101

stasm · 2017-12-09T01:19:42Z

This is the first part of #103. It also fixes #104.

stasm · 2017-12-10T01:50:34Z

This is now ready for reviews. The to-do list at the bottom of #103 has more information about my next steps. For now, I'd like to land this minimal implementation without doing any sanitization of attributes or elements.

There are four commit in this PR. The first one is a small change to how fluent-react's tests work which made it easier to write tests for the second commit. The second commit is where the DOM overlays feature is introduced. The third commit removes MessageContext.formatToParts whose only use-case was fluent-react. The fourth renames FluentType.valueOf to toString because now it really always returns a string. It was called valueOf because it could return other types, like React elements.

@hkasemir I'd love to get your input on this, especially on the second commit.

hkasemir

Looks good to me, with a couple small suggestions/questions.

hkasemir · 2017-12-11T19:14:34Z

fluent-react/examples/html-markup/src/App.js

+    <div>
+      <Localized id="sign-in-or-cancel"
+        button={<button onClick={() => showAlert('clicked-sign-in')}></button>}
+        a={<button className="text" onClick={() => showAlert('clicked-cancel')}></button>}


this is a little confusing that the component included is a button, but the prop name is a and it's a in the fallback string.

I believe this will actually result in two buttons in the final markup? Perhaps we can come up with a more clear convention for how to name components to be overlayed, so that we can allow for multiple links/buttons/etc without causing this confusion.

Yes, the result will be two buttons.

I went for names that are familiar if you know HTML. Otherwise we'd need something like <button1> and <button2>, or <signin> and <cancel>. I'm slightly worried about using non-standard HTML names in the translations because the are more likely to be translated by the localizers.

Let's also keep in mind that it's rare to have two elements of the same type.

hkasemir · 2017-12-11T19:18:39Z

fluent-react/examples/html-markup/src/l10n.js

+clicked-sign-in = You are now signed in.
+clicked-cancel = OK, nevermind.
+
+agree-prompt = My name is <input/> and <button>I agree</button>.


this triggered me to think about the compound message for this element - how could we translate the placeholder? Is that functionality included in this PR, or should we think about it for future feature development?

Great question. It's out of scope of this PR. I put it on the to-do list in #103. We'll need to build a list of known elements and their localizable attributes and we'll also need a way for developers to define which props are localizable on custom components.

hkasemir · 2017-12-11T19:25:47Z

fluent-react/src/localized.js

+
+      return cloneElement(
+        elems[childNode.localName],
+        // XXX Explcitly ignore any attributes defined in the translation.


nit: Explcitly -> Explicitly

hkasemir · 2017-12-11T20:45:37Z

fluent-react/test/localized_overlay_test.js

+    const wrapper = shallow(
+      <Localized id="foo"
+        confirm={<button className="confirm"></button>}
+        cancel={<button className="cancel"></button>}


cool, this is much more clear than the example I called out above. Like I said, perhaps we can do something conventionally to make it clear what the api is doing here with the prop names v the actual components inside.

I like this because it's very semantic, but I'm slightly afraid that localizers also translate the names of elements, like so:

<confirm>Sign in</confirm> or <cancel>cancel</cancel>. <potwierdź>Zaloguj się</potwierdź> albo <anuluj>anuluj</anuluj>.

For this reason I think it's safest to use known HTML names, like button or a.

hkasemir · 2017-12-11T21:11:10Z

fluent-react/test/localized_overlay_test.js

+foo = Click <button>me</button>!
+`)
+
+    const wrapper = shallow(


it's cool to see how you set this up for testing!

hkasemir · 2017-12-11T21:18:22Z

fluent-react/src/localized.js

+    const [args, elems] = toArguments(this.props);
+    const { value, attrs } = l10n.formatCompound(mcx, msg, args);
+
+    if (value === null || !value.includes('<')) {


I suppose there are no unexpected side-effects if the string inlcudes the < character outside of the context of a markup element. For example The result is that 0 < 3 ....

It looks like the parseMarkup that comes next will just leverage the DOM API to split out the different parts, and so this step is more of an optimization in case the translation string does not contain react components. Just thinking out loud and checking my understanding - does that sound about right?

Correct. In the rare scenario of a false-positive (like in the example you gave) we'll pay a bit extra to parse HTML which is not there. It will parse as a text node and things should work just fine. I'll add a test.

zbraniecki

looks great! Couple comments for readability up to your discretion.

zbraniecki · 2017-12-12T02:55:26Z

fluent-react/src/localized.js

-    return this.value;
-  }
-}
+const RESERVED_PROPS = [


can you document where this list comes from and what are reserved props?

zbraniecki · 2017-12-12T03:00:42Z

fluent-react/src/localized.js

+        null,
+        // Void elements (like input) must not have any children. If
+        // childNode.textContent is an empty string we need to pass null
+        // instead.


I struggle to understand the comment.
Why are you testing textContent in the context of void elements, and if it's an empty string you're returning null instead?
May be an indication that other readers will struggle with it as well.

fluent-react's markup overlays (projectfluent#101) removed the dependency on fluent's FluentType which was hardcoded as an import from fluent/compat. Without this dependency all imports from fluent are in the hands of developers again and they can decide to use the ES2015+ or the compat builds as they wish. As long as they do it consistently, regular instanceof checks will work well.

fluent-react's markup overlays (#101) removed the dependency on fluent's FluentType which was hardcoded as an import from fluent/compat. Without this dependency all imports from fluent are in the hands of developers again and they can decide to use the ES2015+ or the compat builds as they wish. As long as they do it consistently, regular instanceof checks will work well.

- Implement Fluent Syntax 0.5. - Add support for terms. - Add support for `#`, `##` and `###` comments. - Remove support for tags. - Add support for `=` after the identifier in message and term defintions. - Forbid newlines in string expressions. - Allow trailing comma in call expression argument lists. In fluent 0.6.x the new Syntax 0.5 is supported alongside the old Syntax 0.4. This should make migrations easier. The parser will correctly parse Syntax 0.4 comments (prefixed with `//`), sections and message definitions without the `=` after the identifier. The one exception are tags which are no longer supported. Please use attributed defined on terms instead. - Add `mapContextAsync`. (#125) This is the async counterpart to mapContextSync. Given an async iterable of `MessageContext` instances and an array of ids (or a single id), it maps each identifier to the first `MessageContext` which contains the message for it. An ordered interable of `MessageContext` instances can represent the current negotiated fallback chain of languages. This iterable can be used to find the best existing translation for a given identifier. The iterable of `MessageContexts` can now be async, allowing code like this: ```js async formatString(id, args) { const ctx = await mapContextAsync(contexts, id); if (ctx === null) { return id; } const msg = ctx.getMessage(id); return ctx.format(msg, args); } ``` The iterable of `MessageContexts` should always be wrapped in `CachedIterable` to optimize subsequent calls to `mapContextSync` and `mapContextAsync`. Because `mapContextAsync` uses asynchronous iteration you'll likely need the regenerator runtime provided by `babel-polyfill` to run the `compat` builds of `fluent`. - Expose the `ftl` dedent helper. The `ftl` template literal tag can be used to conveniently include FTL snippets in other code. It strips the common indentation from the snippet allowing it to be indented on the level dictated by the current code indentation. ```js ctx.addMessages(ftl` foo = Foo bar = Bar ); ``` - Remove `MessageContext.formatToParts`. It's only use-case was passing React elements as arguments to translations which is now possible thanks to DOM overlays (#101). - Rename `FluentType.valueOf` to `FluentType.toString1. Without `MessageContext.formatToParts`, all use-cases for `FluentType.valueOf` boil down to stringification. - Remove `FluentType.isTypeOf`. fluent-react's markup overlays (#101) removed the dependency on fluent's `FluentType` which was hardcoded as an import from fluent/compat. Without this dependency all imports from fluent are in the hands of developers again and they can decide to use the ES2015+ or the compat builds as they wish. As long as they do it consistently, regular instanceof checks will work well.

- Allow limited markup in translations. (projectfluent#101) Translations in Fluent can now include simple HTML-like markup. Elements found in translations will be matched with props passed to `<Localized>`. These props must be React elements. Their content will be replaced by the localizable content found for the corrensponding markup in the translation. This is a breaking change from `fluent-react` 0.4.1. See migration notes below. ```properties send-comment = <confirm>Send</confirm> or <cancel>go back</cancel>. ``` ```js <Localized id="send-comment" confirm={ <button onClick={sendComment}></button> } cancel={ <Link to="/"></Link> } > <p>{'<confirm>Send</confirm> or <cancel>go back</cancel>.'}</p> </Localized> ``` The rendered result will include the props interpolated into the translation: ```js <p> <button onClick={sendComment}>Send</button> or <Link to="/">go back</Link>. </p> ``` When naming markup elements it's possible to use any name which is a valid prop name. Translations containing markup will be parsed using a hidden `<template>` element. It creates a safe inert `DocumentFragment` with a hierarchy of text nodes and HTML elements. Any unknown elements (e.g. `cancel` in the example above) are parsed as `HTMLUnknownElements`. `fluent-react` then tries to match all elements found in the translation with props passed to the `<Localized>` component. If a match is found, the element passed as a prop is cloned with the translated text content taken from the `DocumentFragment` used as `children`. - Filter props with <Localized attrs={{…}}>. (projectfluent#139, projectfluent#141) The `<Localized>` component now requires the `attrs` prop to set any localized attributes as props on the wrapped component. `attrs` should be an object with attribute names as keys and booleans as values. ```jsx <Localized id="type-name" attrs={{placeholder: true}}> <input type="text" placeholder="Localizable placeholder" value={name} onChange={…} /> </Localized> ``` By default, if `attrs` is not passed, no attributes will be set. This is a breaking change compared to the previous behavior: in `fluent-react` 0.4.1 and before `<Localized>` would set _all_ attributes found in the translation. If you're setting localized attributes as props of elements wrapped in `<Localized>`, in `fluent-react` 0.6.0 you'll need to also explicitly allow the props you're interested in using the `attrs` prop. This protects your components from accidentally gaining props they aren't expecting or from translations overwriting important props which shouldn't change. ```jsx // BEFORE (fluent-react 0.4.1) <Localized id="type-name"> <input type="text" placeholder="Localizable placeholder" value={name} onChange={…} /> </Localized> ``` ```jsx // AFTER (fluent-react 0.6.0) <Localized id="type-name" attrs={{placeholder: true}}> <input type="text" placeholder="Localizable placeholder" value={name} onChange={…} /> </Localized> ``` In `fluent-react` 0.4.1 it was possible to pass React elements as _external arguments_ to localization via the `$`-prefixed props, just like you'd pass a number or a date. This was a bad localization practice because it resulted in the translation being split into multiple strings. ```properties send-comment-confirm = Send send-comment-cancel = go back send-comment = { $confirmButton } or { $cancelLink }. ``` ```jsx // Bad practice. This won't work in fluent-react 0.6.0. <Localized id="send-comment" $confirmButton={ <Localized id="send-comment-confirm"> <button onClick={sendComment}>{'Send'}</button> </Localized> } $cancelLink={ <Localized id="send-comment-cancel"> <Link to="/">{'go back'}</Link> </Localized> } > <p>{'{ $confirmButton } or { $cancelLink}.'}</p> </Localized> ``` `fluent-react` 0.6.0 removes support for this feature. It is no longer possible to pass React elements as `$`-prefixed _arguments_ to translations. Please migrate your code to use markup in translations and pass React elements as _props_ to `<Localized>`. In the example above, change `$confirmButton` to `confirm` and `$cancelLink` to `cancel`. Note that you don't need to wrap the passed element in another `<Localized>` anymore. In particular, you don't need to assign a new message id for it. The text for this element will be taken from the `send-comment` message which can now include the markup for the button and the link. ```properties send-comment = <confirm>Send</confirm> or <cancel>go back</cancel>. ``` ```jsx // BEFORE (fluent-react 0.4.1) <Localized id="send-comment" $confirmButton={ <Localized id="send-comment-button"> <button onClick={sendComment}>{'Send'}</button> </Localized> } $cancelLink={ <Localized id="send-comment-cancel"> <Link to="/">{'go back'}</Link> </Localized> } > <p>{'{ $confirmButton } or { $cancelLink}.'}</p> </Localized> ``` ```jsx // AFTER (fluent-react 0.6.0) <Localized id="send-comment" confirm={ <button onClick={sendComment}></button> } cancel={ <Link to="/"></Link> } > <p>{'<confirm>Send</confirm> or <cancel>go back</cancel>.'}</p> </Localized> ``` `fluent-react` 0.6.0 works best with `fluent` 0.6.0. It might still work with `fluent` 0.4.x but passing elements as `$`-prefixed arguments to translations will break your app. You might also run into other issues with translations with attributes and no values. Upgrading your code to [`fluent` 0.6.0][] and your localization files to [Fluent Syntax 0.5][] is the best way to avoid troubles. [`fluent` 0.6.0]: https://github.com/projectfluent/fluent.js/releases/tag/fluent%400.6.0 [Fluent Syntax 0.5]: https://github.com/projectfluent/fluent/releases/tag/v0.5.0

stasm force-pushed the fluent-react-overlays branch 3 times, most recently from 4ee735e to e709f56 Compare December 9, 2017 23:40

stasm mentioned this pull request Dec 10, 2017

DOM fragments localization for fluent-react #103

Closed

stasm force-pushed the fluent-react-overlays branch 2 times, most recently from ebbb048 to d9f10aa Compare December 10, 2017 01:36

stasm requested a review from zbraniecki December 10, 2017 01:39

hkasemir approved these changes Dec 11, 2017

View reviewed changes

zbraniecki approved these changes Dec 12, 2017

View reviewed changes

Remove MessageContext stub from fluent-react's tests

49b283f

stasm force-pushed the fluent-react-overlays branch from d9f10aa to 7cb3935 Compare December 12, 2017 04:23

stasm added 3 commits December 11, 2017 20:33

DOM overlays for fluent-react

087978b

Remove MessageContext.formatToParts

8c419b1

Rename FluentType.valueOf to toString

3036df2

stasm force-pushed the fluent-react-overlays branch from 7cb3935 to 3036df2 Compare December 12, 2017 04:37

stasm merged commit 4967fff into projectfluent:master Dec 12, 2017

stasm deleted the fluent-react-overlays branch December 12, 2017 04:43

stasm mentioned this pull request Dec 12, 2017

Remove FluentType.isTypeOf #108

Merged

stasm mentioned this pull request Dec 19, 2017

fluent-react: Introduce the Placeholder component #49

Closed

jimporter mentioned this pull request Mar 5, 2018

Update fluent and fluent-react to the latest version 🚀 mozilla-lockwise/lockbox-extension#508

Closed

stasm added fluent-react overlay labels Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOM overlays for fluent-react #101

DOM overlays for fluent-react #101

stasm commented Dec 9, 2017 •

edited

Loading

stasm commented Dec 10, 2017

hkasemir left a comment

hkasemir Dec 11, 2017

stasm Dec 12, 2017

hkasemir Dec 11, 2017

stasm Dec 12, 2017 •

edited

Loading

hkasemir Dec 11, 2017

hkasemir Dec 11, 2017

stasm Dec 12, 2017

hkasemir Dec 11, 2017

hkasemir Dec 11, 2017

stasm Dec 12, 2017

zbraniecki left a comment

zbraniecki Dec 12, 2017

zbraniecki Dec 12, 2017

DOM overlays for fluent-react #101

DOM overlays for fluent-react #101

Conversation

stasm commented Dec 9, 2017 • edited Loading

stasm commented Dec 10, 2017

hkasemir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbraniecki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stasm commented Dec 9, 2017 •

edited

Loading

stasm Dec 12, 2017 •

edited

Loading