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

Closing bracket alignment #411

Closed
mike-engel opened this issue Feb 6, 2016 · 4 comments

Comments

@mike-engel
Copy link

commented Feb 6, 2016

Just upgraded to 6.0.1, which has been great and provided even more (missing) consistency. I do feel like the closing bracket rule should either be modified or removed. What I believe the rule is trying to enforce:

// bad
<div>
  Some content</div>

<div>
  Some content
  </div>

// good!
<div>
  some content
</div>

Which is great! But with self-closing tags with a lot of attributes, it gets tricky.

// errors
<Link to={this.props.page === 1 ? 'javascript:void(0)' : `/?page=${this.props.page - 1}`}
  className={this.props.page === 1 ? 'pagination__link disabled' : 'pagination__link'}
  onClick={this.props.page === 1 ? () => {} : this.getMoreBookmarks}
  disabled={this.props.page === 1}
  data-hook='previous'>
    &#x276e; Previous
</Link>

<SearchForm dispatch={props.dispatch}
  history={props.history}
  searchTerm={props.searchTerm} />

This is probably an error with ESLint, but I figured I'd start here first. Thanks!

@feross

This comment has been minimized.

Copy link
Member

commented Feb 7, 2016

Try this instead:

<Link to={this.props.page === 1 ? 'javascript:void(0)' : `/?page=${this.props.page - 1}`}
  className={this.props.page === 1 ? 'pagination__link disabled' : 'pagination__link'}
  onClick={this.props.page === 1 ? () => {} : this.getMoreBookmarks}
  disabled={this.props.page === 1}
  data-hook='previous'
>
  &#x276e; Previous
</Link>
<SearchForm dispatch={props.dispatch}
  history={props.history}
  searchTerm={props.searchTerm}
/>

@feross feross closed this Feb 7, 2016

@feross feross added the question label Feb 7, 2016

@mike-engel

This comment has been minimized.

Copy link
Author

commented Feb 7, 2016

Thanks @feross. I can do that, just looks a bit silly.

@feross

This comment has been minimized.

Copy link
Member

commented Feb 7, 2016

Writing it like this makes more sense, to me:

<Link
  to={this.props.page === 1 ? 'javascript:void(0)' : `/?page=${this.props.page - 1}`}
  className={this.props.page === 1 ? 'pagination__link disabled' : 'pagination__link'}
  onClick={this.props.page === 1 ? () => {} : this.getMoreBookmarks}
  disabled={this.props.page === 1}
  data-hook='previous'
>
  &#x276e; Previous
</Link>
<SearchForm
  dispatch={props.dispatch}
  history={props.history}
  searchTerm={props.searchTerm}
/>
@mike-engel

This comment has been minimized.

Copy link
Author

commented Feb 7, 2016

Yeah, definitely looks better. I'll experiment with it. Thanks again!

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
2 participants
You can’t perform that action at this time.