Create hyperlinks for URLs #339
Conversation
|
There can be multiple elements going into a field if a field is configured to be shown but not grouped by, that's why there is Your code only looks for links at the beginning of field value, why not look for them anywhere in the string with a regex like the code I linked in the issue did? |
You're right. I totally forgot about grouping. Well, there goes my idea of using the
Because allowing URLs in sub-strings introduces the whole hassle of filtering (esp. the "border" cases that you mentioned). I'm not sure if you're willing to merge my simple approach (once it's fixed). But I think it would at least be better than no URL recognition. And it doesn't (shouldn't) introduce a regression. So why not pick the low-hanging fruit now and add sub-string matching later? |
Yeah, that's reasonable and I see what you mean about much more complexity with substrings. I'll test this locally after your fix and merge if there are no issues. |
|
Just added the regex for validity checking. I think it works good. |
InteractiveHtmlBom/web/ibom.js
Outdated
| td = document.createElement("TD"); | ||
| td.innerHTML = highlightFilter(Array.from(valueSet).join(", ")); | ||
| var output = new Array(); | ||
| const urlRegex = /^((file|https?):\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{1,256}\.[a-zA-Z0-9()]{1,6}\b([-a-zA-Z0-9()@:%_\+.~#?&//=]*))$/g; |
There was a problem hiding this comment.
Move this constant to top of the class.
Regex for file:// needs to be relaxed, there are no length limits or dot requirements in file paths, also more characters are allowed like spaces. In general make it separate from http(s). Something like (https?:....|file:....).
This regex is good enough https://gitlab.com/kicad/code/kicad/-/blob/2f8cc845513b918e219f32733a210bf458138f91/kicad/pcm/schemas/pcm.v1.schema.json#L287
There was a problem hiding this comment.
Move this constant to top of the class.
Can you please point me to the class you are referring to. I could not find a JS class.
I just moved the definition to the top of the function. Please get back to me if you prefer a different location.
This regex is good enough https://gitlab.com/kicad/code/kicad/-/blob/2f8cc845513b918e219f32733a210bf458138f91/kicad/pcm/schemas/pcm.v1.schema.json#L287
Looks good at first glance, but I see potential issues. Would you mind reviewing my commit message and maybe consider improving the regex in the original code? See 9c1f97c
As suggested by @qu1ck, use https://gitlab.com/kicad/code/kicad/-/blob/2f8cc845513b918e219f32733a210bf458138f91/kicad/pcm/schemas/pcm.v1.schema.json#L287 as a reference, but fix these issues: * remove the single period (any char) in the http(s) part to allow single-character host names, such as http://a (valid, brings up the default page of the webserver on host 'a') * add missing parenthesis (group) around everything between ^ and $ for the alternative (|) to work as expected (the original regex will match file:// in the middle of the string, as the regex matches anything that *either* *begins* with http(s):// *or* *ends* with a file://... URL scheme)
|
Thanks for looking into this. Please check my latest commits and let me know if you have further comments. |
|
Yeah I misspoke about the class, top of function is fine, I just didn't want it to be reparsed in a loop. Thanks for your contribution! |
2 participants
As discussed in #338, this is my suggestion for adding clickable links to anything that looks like a URL.
Please review and test.
I was not sure about the
join(", ")that was used to concatenate the array into a string (ibom.js:662), as I would expect only a single element to be filled into each cell (at least for "All the other fields", that are not reference designator groups). Maybe a simple[0]would suffice? However, urlifying the joined (comma-separated) list felt wrong, so I chose to urlify each element separately.I am not very experienced in JS, so please take my code with caution. 😉 I'm open to suggestions for improvements.