Generate DOM for markdown links in help text #1979
Conversation
| @@ -181,6 +182,36 @@ class ReportRenderer { | |||
| } | |||
|
|
|||
| /** | |||
| * @param {string} text | |||
| * @return {!Element} | |||
There was a problem hiding this comment.
@return {!HTMLSpanElement}. A function description here would be good too :)
There was a problem hiding this comment.
Is the type of element important We're not specifying subclass type for the other methods.
There was a problem hiding this comment.
Is the type of element important We're not specifying subclass type for the other methods.
not really, though it doesn't hurt. I'm more thinking for documentation purposes it's nice to have the information that your linkified text is coming back in a <span>.
| const element = this._createElement('span'); | ||
|
|
||
| // Split on markdown links (e.g. [some link](https://...)). | ||
| const parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g); |
There was a problem hiding this comment.
any reason to capture fullMarkdownLink if it's going to be unused?
| while (parts.length) { | ||
| // Remove the same number of elements as there are capture groups. | ||
| // eslint-disable-next-line no-unused-vars | ||
| const [preambleText, fullMarkdownLink, linkText, linkHref] = parts.splice(0, 4); |
There was a problem hiding this comment.
if you want to keep the larger capture group, you can also rename fullMarkdownLink to _ to indicate that it's unused and drop the // eslint-disable-next-line no-unused-vars
There was a problem hiding this comment.
ooh, can also do const [preambleText, , linkText, linkHref], though don't know how our eslint would react
There was a problem hiding this comment.
i just removed the extra group. no longer needed.
| const renderer = new ReportRenderer(document); | ||
|
|
||
| describe('createElement', () => { | ||
| it('creates a simple element using default values', () => { |
There was a problem hiding this comment.
what's new and what's not is very confusing when you move things around :P
There was a problem hiding this comment.
i like to keep you on your toes.
| }); | ||
|
|
||
| describe('_convertMarkdownLinksToElement', () => { | ||
| it('converts links', () => { |
There was a problem hiding this comment.
A few more corner cases would be nice. With/without preamble text and text afterwards, etc
| const a = this._createElement('a'); | ||
| a.rel = 'noopener'; | ||
| a.target = '_blank'; | ||
| a.href = linkHref; |
There was a problem hiding this comment.
do we want to verify the url? Maybe someday it would be good if we start using this in more places, but today it would start checking our help text URLs, which would be nice
There was a problem hiding this comment.
what do you mean by verify?
There was a problem hiding this comment.
what do you mean by verify?
sorry, I meant like check that it's a valid url. a.href = new URL(linkHref).href or whatever
| const parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g); | ||
|
|
||
| while (parts.length) { | ||
| // Remove the same number of elements as there are capture groups. |
There was a problem hiding this comment.
I always forget that splice both creates a new array and modifies the original, so maybe change the comment to something like // Pop off the same number of elements as there are capture groups. to make it clear?
There was a problem hiding this comment.
you could also avoid the splice by doing something like
let parts = text.split(/(\[(.*?)\]\((https?:\/\/.*?)\))/g);
while (parts.length) {
const [preambleText, fullMarkdownLink, linkText, linkHref, ...rest] = parts;
// ...
parts = rest;
}more explicit, though the extra assignment is kind of annoying
There was a problem hiding this comment.
updated the comment.
|
PTAL |
| a.href = (new URL(linkHref)).href; | ||
| element.appendChild(a); | ||
| } catch (e) { | ||
| element.appendChild(this._document.createTextNode(linkText)); |
There was a problem hiding this comment.
should we fail silently here? Seems like a big enough mistake that it should complain loudly and end up in _renderException
There was a problem hiding this comment.
I think it's better to fail silently here. If a link is busted for some reason, it shouldn't block the whole report from being generated.
There was a problem hiding this comment.
but why is the link busted? Right now, at least, they're all coming from helpText. An error in one of those should be fixed immediately
There was a problem hiding this comment.
done.
FWIW, b/c of the regex, the only time we'd get an invalid URL is if someone wrote https://. or used something crazy in help text like [boom](🍣🍺).
|
This ready. Let's get it landed so other PRs can proceed. |
R: all
Just support in help text / descriptions.
Will add a test once #1963 lands.