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

Convert the homepage to React #668

Merged
merged 3 commits into from Sep 6, 2018
Merged

Conversation

mwiencek
Copy link
Member

@mwiencek mwiencek commented May 14, 2018

The ArtworkImage commit is cherry-picked from the entity sidebar PR.

@yvanzo
Copy link
Contributor

yvanzo commented May 14, 2018

Maybe we can use a Markdown renderer to support emphasis? This will be needed for OTHER-77.

@mwiencek
Copy link
Member Author

@yvanzo Maybe a limited subset for emphasis only? I'm even thinking we'd want to support either asterisks or underscores for emphasis but not both, to decrease the syntax burden for translators. A full markdown renderer would create a pretty big change in semantics for all of our source strings, so I'm not sure about it yet. I could see how it would unlock some possibilities for translating larger pieces of text.

For now, we could also change the React interpolation code to recognize <strong> tags in source strings, and output the right thing. Then I can leave the strings unchanged. What do you think is a good course of action for this PR?

@yvanzo
Copy link
Contributor

yvanzo commented May 15, 2018

@mwiencek: For this PR, recognizing <strong> tags in source strings, definitely. 👍

For the record, source strings currently contain:

  • 40 <strong> (and <b>)
  • 21 <code>
  • 17 <span> (with specific class names)
  • 8 <em>
  • 7 <a> (mostly from relationships)
  • 3 <li> and 1 <ul> (from relationships)
  • (And 2 <abbr> (with title) that can probably be moved out of strings)

@mwiencek mwiencek force-pushed the homepage-react branch 2 times, most recently from e0735f3 to 1260fdb Compare September 3, 2018 06:21
@mwiencek
Copy link
Member Author

mwiencek commented Sep 3, 2018

Okay, this took a century but there's a new i18n string parser here which can convert HTML into React elements. The old expand code is still there and used, particularly by our knockout stuff - the new one can primarily only be used with React.

I reverted all of the changes I made to the strings on the homepage.

Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

root/static/scripts/tests/i18n/expand2.js Show resolved Hide resolved
We have a good number of i18n strings which contain HTML, and it'd be
very useful to continue supporting that. However, it's difficult with
React: in order to embed raw HTML into JSX, you have to use a prop
called `dangerouslySetInnerHTML`. That's not a nice API to use
everywhere we want to display a simple string. Well, say we wrote a
small helper utility that used `dangerouslySetInnerHTML` for us. This
also wouldn't work out well: you can only use `dangerouslySetInnerHTML`
on an actual element, so we'd have to embed many, many strings in a
useless <span />. Yuck.

Clearly, we need a way to parse HTML in our strings and convert them
into React elements. There's no way around that. The new `expand`
implementation (a simple recursive-descent parser) does that.

There are some "breaking" changes in the new implementation:

  1. You can no longer interpolate raw HTML into a string; it has to be
     a React element. All string values will be HTML-escaped by React.

  2. Only a safe subset of HTML is supported.

  3. An (apparently unused) feature of the link substitution syntax was
     removed. Previously, if you had "{foo|bar}" and `bar` was a key in
     the substitution args, it would use the value at that key as the
     link content. This has been removed because it doesn't appear to be
     used, and it makes it ambiguous or difficult to determine if `bar`
     is a key or a text string in some cases. You now have to be
     explicit and use "{foo|{bar}}". This method was already supported
     and used.

The new function is now used in place of the old one whenever `__react`
is set in the substitution args. The old "expand to array" behavior of
the old function has thus been removed.
@mwiencek mwiencek merged commit d36517a into metabrainz:master Sep 6, 2018
@mwiencek mwiencek deleted the homepage-react branch September 6, 2018 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants