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

fix(comment) comments via email notification #227

Closed
wants to merge 5 commits into from

Conversation

garrettbryan
Copy link
Contributor

Github sends emails to users who are subscribed to
notifications. The Gitalk comment behavior should match that of
Github.

The previous behavior does not hide extended email replys.
Often times the email response contains the complete email
chain. The visible email chain confuses the user with
unformatted html strings.

Incorrect comment formatting for email replies. #224

Before submitting a pull request, please make sure the following is done:

  1. Fork the repository and create your branch from master.
  2. If you've added code that should be tested, add tests!
  3. If you've changed APIs, update the documentation.
  4. Ensure the test suite passes (npm test).
  5. Make sure your code lints (npm run lint).
  6. Commit your changes (git commit) Commit Message Format Reference.

Github sends emails to users who are subscribed to
notifications. The Gitalk comment behavior should match that of
Github.

The previous behavior does not hide extended email replys.
Often times the email response contains the complete email
chain. The visible email chain confuses the user with
unformatted html strings.

Incorrect comment formatting for email replies. gitalk#224
@coveralls
Copy link

coveralls commented Jan 30, 2019

Coverage Status

Coverage decreased (-3.5%) to 95.575% when pulling 15565ab on garrettbryan:fix-email-comments into 7f04a89 on gitalk:master.

@garrettbryan
Copy link
Contributor Author

Hi guys,
Thought I'd take a crack at this fix. I'm looking forward to hearing back!

@garrettbryan
Copy link
Contributor Author

garrettbryan commented Feb 1, 2019

If you take a look at this issue, I opened a case with Github support about the json that appears in the html string. I'll let you know what they say. It might be that this work is unnecessary, if Github comes back and admits the json shouldn't be included in the html string.

@booxood
Copy link
Collaborator

booxood commented Feb 11, 2019

@garrettbryan Good job. Please don't commit the dist directory. When release the the dist directory will been updated and committed.

@garrettbryan
Copy link
Contributor Author

@booxood sorry for the delay. I've been focused on another project. Sorry about committing the dist directory. I'll clean it up and try again.

@garrettbryan
Copy link
Contributor Author

If you take a look at this issue, I opened a case with Github support about the json that appears in the html string. I'll let you know what they say. It might be that this work is unnecessary, if Github comes back and admits the json shouldn't be included in the html string.

@booxood I heard from GitHub support, they confirmed that comments via email should contain the extra json data, hidden by the button.

@garrettbryan
Copy link
Contributor Author

@booxood one other question, How concerned are you with the tests? I haven't fully implemented the test cases.

refactor email comment code to not use findDOMNode to register click
listener.

add test for extended email comment processing.

Incorrect comment formatting for email replies. gitalk#224
refactor email comment code to not use findDOMNode to register click
listener.

add test for extended email comment processing.

Incorrect comment formatting for email replies. gitalk#224
@garrettbryan
Copy link
Contributor Author

garrettbryan commented Feb 21, 2019

I'm sorry about muddying up this pull request!
I'm going to work out the tests and try again.

@draveness
Copy link

Hi @garrettbryan, thanks for the great work. Do we have any progress on this issue?

@booxood
Copy link
Collaborator

booxood commented Feb 16, 2020

@garrettbryan Merged and little modified test case. 😌

@booxood booxood closed this Feb 16, 2020
@garrettbryan
Copy link
Contributor Author

garrettbryan commented Feb 20, 2020 via email

@songzhonghou songzhonghou mentioned this pull request Feb 21, 2020
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.

None yet

4 participants