Hook up the details page to data #912
Conversation
|
@muffinresearch I bumbled through this one, let me know how it looks. I have not manually tested the install button since I'm not sure what |
src/amo/components/AddonDetail.js
Outdated
| const { i18n, addon } = this.props; | ||
| const authorList = []; | ||
| for (const author of addon.authors) { | ||
| authorList.push(<a key={author.url} href={author.url}>{author.name}</a>); |
There was a problem hiding this comment.
This feels like it should be a ul since its a list of something. You could then add the comma with generated content.
There was a problem hiding this comment.
Ah now I see it's in an h1, in which case it can't be a ul. Ignore the previous comment.
src/amo/components/AddonDetail.js
Outdated
| <span className="author">by <a href="#">AwesomeAddons</a></span></h1> | ||
| <InstallButton slug="placeholder" /> | ||
| <h1> | ||
| {addon.name} <span className="author">{i18n.gettext('by')} {authorList}</span> |
There was a problem hiding this comment.
@muffinresearch I almost forgot to localize this. Is it ok to isolate the by like this? It seems like creating a longer format string would be complicated because of the span tag.
There was a problem hiding this comment.
Good catch!
If we only translate by the downside would be if a locale needed to change the word order for some reason it couldn't.
In the past we've done things like this to be able to expose the full context for translation allthought for disco pane the string was built on the server.
{i18n.sprintf(i18n.gettext('%(addonName)s %(startSpan)sby %(authorList)s%(endSpan)s'), { addonName, authorList, startSpan, endSpan })}If passed through dompurify if the tags are reversed via the translation you will still get a sane output albeit the markup won't be wrapping the things it should, but you will at least get valid HTML.
e.g:
</span>foo<span>becomes:
foo<span></span>
|
@muffinresearch this is ready for another look |
Yeah it won't magically work since the APIs aren't going to be exposed to non-disco pane hosts. |
| }); | ||
|
|
||
| it('converts \r\n\r\n to <br/>', () => { | ||
| assert.equal(nl2br('\r\n\r\n'), '<br /><br />'); |
There was a problem hiding this comment.
Should this normalize multiple sets of line breaks to a single <br /> like the description string suggests?
|
Gave this a spin and it looks good. r+wc |
|
@muffinresearch if you have a mo' could you check that the new title localization is sane? |
src/amo/components/AddonDetail.js
Outdated
| } | ||
|
|
||
| const authorList = addon.authors.map( | ||
| (author) => `<a href=${author.url}>${author.name}</a>`); |
There was a problem hiding this comment.
Missing quotes on the attr.
There was a problem hiding this comment.
d'oh. Refactoring casualty. Thanks.
2 participants
Fixes mozilla/addons#9794