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 issues #2644 and #2662 #2712

Merged
merged 4 commits into from
Jan 7, 2019
Merged

fix issues #2644 and #2662 #2712

merged 4 commits into from
Jan 7, 2019

Conversation

roottool
Copy link
Contributor

Description

Solved #2644 and #2662.
fix-issues-2644-and-2662
The reason for the bug was that inserting unclosed HTML tags into innerHTML was automatically closed.
So I modified to use the existing escapeHtmlCharacters method only within the ``` range when in "Allow dangerous html tags" mode.

I referred to the following pages.

Issue fixed

#2644 and #2662

Type of changes

  • πŸ”˜ Bug fix (Change that fixed an issue)
  • βšͺ Breaking change (Change that can cause existing functionality to change)
  • βšͺ Improvement (Change that improves the code. Maybe performance or development improvement)
  • βšͺ Feature (Change that adds new functionality)
  • βšͺ Documentation change (Change that modifies documentation. Maybe typo fixes)

Checklist:

  • πŸ”˜ My code follows the project code style
  • βšͺ I have written test for my code and it has been tested
  • βšͺ All existing tests have been passed
  • πŸ”˜ I have attached a screenshot/video to visualize my change if possible

Note

I couldn't run the tests.
That's because the following error code has occurred when I executed npm run test with Powershell included in Windows 10 Pro.

β€»('PWD' is an internal command or external command,
It is not recognized as an operable program or batch file.)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! boost@0.11.11 test: `PWD=$(pwd) NODE_ENV=test ava --serial`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the boost@0.11.11 test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

The part of β€» was translated by Google translation on what was displayed in Japanese.

@ZeroX-DG ZeroX-DG added the awaiting review ❇️ Pull request is awaiting a review. label Dec 17, 2018
@ZeroX-DG
Copy link
Member

Can you tell me the reason why you put the escapeHTML function in the attachmentManagement file?

@roottool
Copy link
Contributor Author

Because I refer to fixLocalURLS function in the attachmentManagement file.
I looked at the fixLocalURLS function to convert HTML, and I decided to put the escapeHTML function in the attachmentManagement file.

If the place to define the function is the best in MarkdownPreview.js, I will rewrite the code.
In that case probably I think that I will define it on line 397 (between fixDecodedURI function and getScrollBarStyle function).
If there is a better place than that place, I would like you to tell me the place.

@ZeroX-DG
Copy link
Member

@roottool line 397 sounds good, please rewrite your code.

@roottool
Copy link
Contributor Author

@ZeroX-DG Thank you for review.
I rewrote my code. Please check it again.

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM πŸŽ‰

@ZeroX-DG ZeroX-DG added approved πŸ‘ Pull request has been approved by sufficient reviewers. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 30, 2018
@Rokt33r Rokt33r added next release (v0.11.13) and removed approved πŸ‘ Pull request has been approved by sufficient reviewers. labels Jan 7, 2019
@Rokt33r Rokt33r merged commit 0289caa into BoostIO:master Jan 7, 2019
@roottool roottool deleted the fix-issue#2644-and-#2662 branch January 7, 2019 05:18
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

3 participants