-
Notifications
You must be signed in to change notification settings - Fork 28
Add a sanitize package #12
Add a sanitize package #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also do for a README.md
packages/sanitize/src/index.js
Outdated
* @return Sanitized text. False on failure. | ||
*/ | ||
export function sanitizeText( text ) { | ||
var _text = wp.utils.stripTags( text ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle this dependency? Ideally it'd be referenced by Node module, but at the very least we need to ensure that the corresponding script handle is enqueued whenever the sanitize package is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, yea - good catch. i'll switch this to referencing as a node module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been refactored and is now imported
packages/sanitize/src/index.js
Outdated
*/ | ||
export function sanitizeText( text ) { | ||
var _text = wp.utils.stripTags( text ), | ||
textarea = document.createElement( 'textarea' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we reuse a shared reference of a textarea element? Is there any overhead to creating a new element every time this is invoked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure there is some overhead and I like that suggestion, would that possible get destroyed though in a SPA context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calypso uses a single shared reference without any trouble:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 4eb3d86
packages/sanitize/src/index.js
Outdated
|
||
try { | ||
textarea.innerHTML = _text; | ||
_text = wp.utils.stripTags( textarea.value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential optimization to consider is bypassing this step if there are no encoded entities to convert.
See related: Automattic/wp-calypso#9725
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, for now the code here exactly matches what we have in core.
packages/sanitize/src/index.js
Outdated
/** | ||
* Strip HTML tags. | ||
* | ||
* @param {string} text Text to have the HTML tags striped out of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: striped
should be stripped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6c93f80
packages/sanitize/src/index.js
Outdated
* | ||
* @return Sanitized text. False on failure. | ||
*/ | ||
export function sanitizeText( text ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a better name and description. sanitizeText
- in what way, against what? convert HTML entities
- into what?
If I read this correctly, I think the answers are "remove tags and encode HTML entities". I'm having a hard time thinking of a good function name, though. Maybe we should just create the encodeHTMLEntities
building block first, and not worry about the combination of the two functions until we need it? This would also be more easily testable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the combined function to maintain existing functionality in PressThis. I'll try to improve the naming and descriptions here. The unit tests would also help make the usages clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the inline docs a bit to hopefully clear up how these functions work.
When are these functions expected to be used? In particular the combination of "strip tags and encode HTML entities" seems a bit strange to me. |
…ari-voiceover Append a no-break space to repeated messages.
I've added a handful of basic tests for |
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
===========================================
- Coverage 100% 87.93% -12.07%
===========================================
Files 6 9 +3
Lines 48 58 +10
Branches 7 10 +3
===========================================
+ Hits 48 51 +3
- Misses 0 5 +5
- Partials 0 2 +2
Continue to review full report at Codecov.
|
|
I just added a Commit is at WordPress/gutenberg@f08c5f4 , if it's useful I'm happy to work on contributing it here. |
@notnownikki that does seem useful, not sure it belongs with |
Issue moved to WordPress/gutenberg #7824 via ZenHub |
These could use some unit tests.