Skip to content

Conversation

muffinresearch
Copy link
Contributor

@muffinresearch muffinresearch commented Jul 3, 2017

Fixes mozilla/addons#10524

This configures addons-frontend to use https://github.com/mozilla/eslint-config-amo/

Once this lands all related eslint prs from greenkeeper can be closed.

Copy link
Contributor

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

I didn't want to comment on every one but where it's obvious it would be nicer to keep every attribute on its own line when they're multiline (including the first attribute after the component/tag name) and to put the closing > on its own line.

Very cool, I like this 👍

r+wc

<Rating className="AddonMeta-Rating"
rating={averageRating}
readOnly
styleName="small" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note that I think these multi-line tags look nicer/align better like:

<Rating className="AddonMeta-Rating"
  rating={averageRating}
  readOnly
  styleName="small"
/>

it's actually what we (I?) have been doing recently. If eslint is cool with that I'd say change it to that while we're already changing these.

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 rule relates to props on the multiline - it doesn't cover closing > or /> afaik. Will check if there's another rule that can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

)} className="AddonDescription">
<ShowMoreCard
header={i18n.sprintf(i18n.gettext('About this %(addonType)s'), { addonType })}
className="AddonDescription">
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

{categories.map((category, index) => (
<li className="Categories-list-item" key={`category-${index}`}>
{categories.map((category) => (
<li className="Categories-list-item"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we have newlines in tags at all we should put every attribute on its own line.

<li className="Categories-list-item" key={`category-${index}`}>
{categories.map((category) => (
<li className="Categories-list-item"
key={category.name}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

</li>
<li>
<a href="#desktop" className="Footer-link Footer-desktop"
<a href="#desktop"
Copy link
Contributor

Choose a reason for hiding this comment

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

All attrs on newline would be good.

close={this.onClose}
items={formatPreviews(previews)}
options={PHOTO_SWIPE_OPTIONS}
thumbnailContent={thumbnailContent} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

currentPage={page}
count={count}
pathname={pathname}
queryParams={queryParams} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

hasSearchParams={hasSearchParams}
loading={loading}
pathname={pathname}
results={results} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

action={`/${api.lang}/${api.clientApp}${pathname}`}
onSubmit={this.handleSearch}
className="SearchForm-form"
ref={(ref) => { this.form = ref; }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

'SearchSortLink--active': currentSort === sort,
})}>
<Link to={sortURL}
className={classNames('SearchSortLink', { 'SearchSortLink--active': currentSort === sort })}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here for the ending > alignment.

"mocha": true,
},
"extends": "airbnb",
"extends": "amo",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is outside the scope of this PR, but we should rename this. I've filed: mozilla/eslint-config-amo#7

@muffinresearch muffinresearch merged commit 9c192be into mozilla:master Jul 3, 2017
@muffinresearch muffinresearch deleted the add-amo-eslint-config branch July 3, 2017 15:45
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.

Use amo eslint config for addons-frontend
2 participants