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.

Sign up for GitHub

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

Jump to bottom

Fixes #3295 - Fetch and render existing issue comments as HTML #3308

Merged
merged 14 commits into from
May 20, 2020
Merged

Fixes #3295 - Fetch and render existing issue comments as HTML #3308

merged 14 commits into from
May 20, 2020

Conversation

miketaylr
Copy link
Member

@miketaylr miketaylr commented May 15, 2020 •
edited
Loading

TODO:

  • write some tests for the modified behavior of get_response_headers
  • remove the entire client-side part of adding a new comment, and handle the XHR bits related to that
  • add more tests
  • fix the tests i broke
  • fix the NSFW handling stuff that's broken now
  • fix busted unit tests
  • add some kind of ajax spinner when submitting a comment
  • remove client-side markdown stuff

@miketaylr miketaylr force-pushed the issues/3295/1 branch 3 times, most recently from cf63e11 to e9533e3 Compare May 16, 2020 01:35
@miketaylr
Copy link
Member Author

So I decided to just do away with the CommentsView and Collection and Model, etc. Instead we'll go old-school, and after doing an XHR post for new comments, we just append the HTML the server sends back to us.

The cool thing about this is it means we no longer need the Markdown client stuff, so we can delete a ton of code here.

miketaylr
miketaylr commented May 17, 2020
View reviewed changes
@miketaylr miketaylr force-pushed the issues/3295/1 branch 2 times, most recently from c482f91 to ced813d Compare May 18, 2020 22:29
@miketaylr miketaylr marked this pull request as ready for review May 18, 2020 23:02
@miketaylr
Copy link
Member Author

OK, I think this is ready for review now.

r? @karlcow
r? @ksy36

@miketaylr miketaylr requested review from karlcow and ksy36 May 18, 2020 23:03
@miketaylr
Copy link
Member Author

🥳 Screen Shot 2020-05-18 at 6 03 36 PM

@miketaylr miketaylr force-pushed the issues/3295/1 branch 3 times, most recently from 9b7f9dc to 55535e2 Compare May 19, 2020 20:12
karlcow
karlcow requested changes May 20, 2020
View reviewed changes
Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

yoohoo. Wunderbar. Thanks @miketaylr for working on this. we are approaching a better system.

Mike Taylor added 8 commits May 20, 2020 15:31
Issue #3295 - Add a loading indicator for comment submission
2052c01
Issue #3295 - Update issue 100 comment fixtures
7796218 
This file will be returned when POSTing a new comment in test mode,
so our existing functional tests can pass. When doing a GET, it will
serve /tests/fixtures/api/issues/100/comments.38e6a9ddcea3ef150105b48bcd1c18f3.json
because it has a ?per_page=100 param.
Issue #3295 - Make api_request and proxy_request the mocked responses
dd802ca
Issue #3295 - Unbreak NSFW image handling
ca0140e
Issue #3295 - Get unit tests happy after mocking proxy_request and ap…
09c88f2 
…i_request
Issue #3295 - Add tests for get_response_headers
4666ba2
Issue #3295 - Remove markdown parser
35e812b 
Now we rely on everything coming back from GitHub before rendering,
so there's no more need for this.
Issue #3295 - Remove dead code conditional
d221e9d
@miketaylr miketaylr force-pushed the issues/3295/1 branch 2 times, most recently from f4fccb0 to c9c81eb Compare May 20, 2020 20:38
@miketaylr
Copy link
Member Author

miketaylr commented May 20, 2020 •
edited
Loading

OK @karlcow, I believe I addressed everything!

Thanks to you and @ksy36 for the review -- this is in better shape as a result.

r? @karlcow

karlcow
karlcow approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@karlcow karlcow left a comment

Choose a reason for hiding this comment

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

thanks @miketaylr 🦞 Let's 🎈

@karlcow karlcow merged commit 3df0c39 into master May 20, 2020
@karlcow karlcow deleted the issues/3295/1 branch May 20, 2020 21:44
@miketaylr
Copy link
Member Author

yay!

@ksy36 ok, you can rebase your form-v2 branch, promise i won't touch build configs for a while ^__^

@ksy36
Copy link
Contributor

ksy36 commented May 21, 2020

sounds good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karlcow karlcow karlcow approved these changes

@ksy36 ksy36 ksy36 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miketaylr @ksy36 @karlcow