Skip to content

Fix #1058 - Display all comments for an issue (not just 30)#1147

Merged
miketaylr merged 3 commits into
webcompat:masterfrom
deepthivenkat:issues/1058/2
Aug 1, 2016
Merged

Fix #1058 - Display all comments for an issue (not just 30)#1147
miketaylr merged 3 commits into
webcompat:masterfrom
deepthivenkat:issues/1058/2

Conversation

@deepthivenkat

Copy link
Copy Markdown
Member

…n 30

@deepthivenkat

Copy link
Copy Markdown
Member Author

r? @miketaylr

Comment thread webcompat/static/js/lib/issues.js Outdated
var _id = $(location.hash);
window.scrollTo(0, _id.offset().top);
}
(response[0].lastPageNumber > 1) ? this.getRemainingComments(++response[0].lastPageNumber) : '';

This comment was marked as abuse.

@miketaylr

miketaylr commented Jul 29, 2016

Copy link
Copy Markdown
Member

@deepthivenkat please run grunt eslint before submitting a PR, there's a number of style problems here:

🐓 grunt eslint
Running "eslint:target" (eslint) task

/Users/miket/dev/compat/webcompat.com/webcompat/static/js/lib/models/comment.js
  28:68   error  Expected linebreaks to be 'LF' but found 'CRLF'  linebreak-style
  28:68   error  Missing semicolon                                semi
  28:68   error  Irregular whitespace not allowed                 no-irregular-whitespace
  30:67   error  Expected linebreaks to be 'LF' but found 'CRLF'  linebreak-style
  30:67   error  Irregular whitespace not allowed                 no-irregular-whitespace
  32:113  error  Expected linebreaks to be 'LF' but found 'CRLF'  linebreak-style
  32:113  error  Irregular whitespace not allowed                 no-irregular-whitespace
  35:12   error  Keyword "if" must be followed by whitespace      space-after-keywords
  35:66   error  Missing space before opening brace               space-before-blocks
  35:67   error  Expected linebreaks to be 'LF' but found 'CRLF'  linebreak-style
  35:67   error  Irregular whitespace not allowed                 no-irregular-whitespace
  37:33   error  Strings must use singlequote                     quotes

✖ 12 problems (12 errors, 0 warnings)

Warning: Task "eslint:target" failed. Use --force to continue.

(I'm assuming that since grunt failed, maybe you didn't run tests again locally either?)

@miketaylr

Copy link
Copy Markdown
Member

As for the commit messages, this changset is re-introducing the fix for #1058, so it should reflect that (not just the difference between the last PR and this one).

@deepthivenkat

Copy link
Copy Markdown
Member Author

I have modified the first commit message to represent all the changes made in the commit.

r? @miketaylr

@miketaylr

Copy link
Copy Markdown
Member

When you add that comment, LGTM.

@miketaylr miketaylr changed the title Fix #1058 - Link header change when comments per issue is less tha… Fix #1058 - Display all comments for an issue (not just 30) Aug 1, 2016
@miketaylr

Copy link
Copy Markdown
Member

@deepthivenkat ping me when you've pushed that comment and I can merge (and so GitHub will send me an email. ^_^)

…e changes, adding comments for test case.
@miketaylr

Copy link
Copy Markdown
Member

Tested locally, this is working now.

Issue with multiple comment pages:
screen shot 2016-08-01 at 1 26 17 pm

Issue with less than 30 comments:
screen shot 2016-08-01 at 1 30 44 pm

@miketaylr

Copy link
Copy Markdown
Member

Aw dang.

$ pep8 --ignore=E402 webcompat/ tests/ config/secrets.py.example

tests/test_api_urls.py:75:80: E501 line too long (103 > 79 characters)

tests/test_api_urls.py:77:80: E501 line too long (104 > 79 characters)

tests/test_api_urls.py:78:80: E501 line too long (102 > 79 characters)

tests/test_api_urls.py:79:80: E501 line too long (92 > 79 characters)

tests/test_api_urls.py:82:80: E501 line too long (98 > 79 characters)

tests/test_api_urls.py:83:80: E501 line too long (100 > 79 characters)

tests/test_api_urls.py:84:80: E501 line too long (96 > 79 characters)

The command "pep8 --ignore=E402 webcompat/ tests/ config/secrets.py.example" failed and exited with 1 during .

@deepthivenkat next time be sure to run pep8 as well when touching python code, thanks!

@miketaylr

Copy link
Copy Markdown
Member

(my bad for not noticing during review)

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.

2 participants