Consolidate redLinks #37
Conversation
| return fragment | ||
| } | ||
|
|
||
| describe('DominoDocumentFragment', () => { |
There was a problem hiding this comment.
I should probably split this test out to its own TestDominoDocumentFragment.js file...
Speaking of, could we have tests only run on files in test subdirectories if the files themselves start with the word test?
|
Reminder to self! Update this PR to adhere to new test file naming convention. |
src/transform/Redlinks.js
Outdated
| * @return {void} | ||
| */ | ||
| const hideRedlinks = (document, fragment) => { | ||
| const spanTemplate = spanToClone(document) |
There was a problem hiding this comment.
Have you considered calling spanToClone() something like newRedlinkTemplate()? Otherwise it sounds kind of weird out of context like, "why would I want to clone a span?" Then you could rename the wordy configureSpanAsAnchorReplacement() as configureRedlinkTemplate() or populateRedlinkTemplate().
There was a problem hiding this comment.
I like it! Fixed!
src/transform/Redlinks.js
Outdated
| @@ -0,0 +1,63 @@ | |||
| /** | |||
There was a problem hiding this comment.
As far as I can tell, "red links" is two words and so the capitalization so the middle l should be capitalized. e.g., "RedLinks" or "redLinks".
src/transform/Redlinks.js
Outdated
| const content = fragment !== undefined ? fragment : document | ||
| redLinkAnchorsInContent(content) | ||
| .forEach(redLink => { | ||
| const span = spanTemplate.cloneNode(false) |
There was a problem hiding this comment.
Why not just create the element each time? Are you seeing a visible performance difference?
There was a problem hiding this comment.
Trying to only touch the document object when absolutely necessary. Also, in this case a shallow clone is all we need.
test/transform/Redlinks.test.js
Outdated
| assert.ok(item2.tagName === 'SPAN') | ||
| assert.ok(item2.innerHTML === '<b>2</b>') | ||
| }) | ||
| it('should hide the expected redlinks in a document fragment', () => { |
There was a problem hiding this comment.
Would you mind using the same single line of space between it() as you did in WidenImage.test.js or update all the tests?
| /** | ||
| * Gets a document fragment suitable for use in unit tests | ||
| * Details: https://github.com/fgnass/domino/issues/73#issuecomment-200466430 | ||
| * @param {?String} HTMLString An html string |
There was a problem hiding this comment.
Missing capitalization of "HTML" :]
| assert.ok(firstChild.tagName === 'DIV') | ||
| assert.ok(secondChild.tagName === 'A') | ||
| }) | ||
| it('should not see unexpected method in Domino DocumentFragment', () => { |
There was a problem hiding this comment.
Consider changing this to something like Domino DocumentFragments should not implement Document.createElement
| it('should not see unexpected method in Domino DocumentFragment', () => { | ||
| const frag = | ||
| documentFragmentFromHTMLString('<div>Oh look! A fragment!</div><a id="link1"></a>') | ||
| assert.ok(frag.createElement === undefined) |
There was a problem hiding this comment.
Since we only want to ensure that createElement() does not exist, consider just using the not operator (!frag.createElement).
There was a problem hiding this comment.
Fixed! With accompanying grumbles about weak typing / implicit type conversions :)
| const frag = | ||
| documentFragmentFromHTMLString('<div>Oh look! A fragment!</div><a id="link1"></a>') | ||
| assert.ok(frag.childNodes.length === 2) | ||
| const firstChild = frag.childNodes[0] |
There was a problem hiding this comment.
Mega style nerd: isolating frag.childNodes[0] seems obvious enough as it is that it's the first child and it only has one usage. Consider dropping firstChild and secondChild for readability.
There was a problem hiding this comment.
Ah good catch. Fixed!
| }) | ||
| it('should return a Domino DocumentFragment with expected childNodes', () => { | ||
| const frag = | ||
| documentFragmentFromHTMLString('<div>Oh look! A fragment!</div><a id="link1"></a>') |
There was a problem hiding this comment.
Consider replacing globally replacing <div>Oh look! A fragment!</div> with <div>text</div> to fit this statement on one line.
| const documentFragmentFromHTMLString = HTMLString => { | ||
| const document = domino.createDocument() | ||
| const template = document.createElement('template') | ||
| template.innerHTML = HTMLString |
There was a problem hiding this comment.
I've been using camelCase on initialisms too. Can you change this to htmlString?
src/transform/RedLinks.js
Outdated
| @@ -0,0 +1,63 @@ | |||
| /** | |||
| * Configures span to be suitable replacement for redLink anchor. | |||
There was a problem hiding this comment.
Since this is English, not code, can we say "red link" in the comments?
There was a problem hiding this comment.
Oh sure! Just a sec...
| const frag = | ||
| documentFragmentFromHTMLString('<div>text</div><a id="link1"></a>') | ||
| assert.ok(frag instanceof domino.impl.DocumentFragment) | ||
| }) |
There was a problem hiding this comment.
Consider adding some new lines between it() here too and also between consecutive describe statements as done in WidenImage.test.js.
There was a problem hiding this comment.
Done! I wonder if we can update linting to enforce our preference here... too easy to miss 'em.
No description provided.