Skip to content

Conversation

muffinresearch
Copy link
Contributor

Fixes mozilla/addons#9568
Fixes mozilla/addons#9563
Fixes mozilla/addons#9562

  • Moves to using JSX for the HTML

TODO: As a follow-up will need to wrap the search code when login is required in the App component so that it gets the App title.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 188eb5c on muffinresearch:better-html into e50f50b on mozilla:master.

.eslintrc Outdated
"object-curly-spacing": 0,
"space-before-function-paren": [2, "never"],
"react/prefer-stateless-function": 0,
"arrow-body-style": "off",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to turn this off. I think it's a good rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's ok some of the time - other times I feel like it's annoying if the lack of braces and a return obscures the readability. I feel like this is one where being opionated gets in the way and I'd rather devs decide what makes sense on a case by case basis.

The reason for disabling it was because it can't deal with the abiguity of returning an object since braces and the start of the object are both { - but I'll check and see if there's extra options to deal with this case before removing it entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the object case you need to wrap it in parens. It seems a little strange but I don't think it looks too bad.

const getConfig = () => ({
  some: 'config',
});

@mstriemer
Copy link
Contributor

Thanks for cleaning this up! Looks good.

r+wc

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 23c12ff on muffinresearch:better-html into e50f50b on mozilla:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3455444 on muffinresearch:better-html into e50f50b on mozilla:master.

@muffinresearch muffinresearch merged commit 13f8dcb into mozilla:master May 4, 2016
@muffinresearch muffinresearch deleted the better-html branch May 4, 2016 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Currently server tests are outputting all CSS + script for both apps Move HTML for main app to jsx Define specific HTML metadata per app
3 participants