Issue 51432: LKSM: special character in sample names is not working well on various pages #1658
Conversation
# Conflicts: # packages/components/package-lock.json # packages/components/package.json # packages/components/releaseNotes/components.md
# Conflicts: # packages/components/releaseNotes/components.md
# Conflicts: # biologics/package-lock.json # biologics/package.json # inventory/package-lock.json # inventory/package.json # sampleManagement/package-lock.json # sampleManagement/package.json
XingY
changed the title
| ### version 6.X | ||
| *Released*: X 2024 | ||
| - Issue 51432: LKSM: special character not working well on various pages | ||
| - Warn user about missing quotes when pasting data into editable grid that containing comma |
There was a problem hiding this comment.
| - Warn user about missing quotes when pasting data into editable grid that containing comma | |
| - Warn user about missing quotes when pasting data into editable grid that contains a comma |
| function lookupValidationError(value: string | number | boolean, fromPaste?: boolean): CellMessage { | ||
| let suffix = ''; | ||
| if (fromPaste && typeof value === 'string' && value.toString().indexOf(',') > -1) { | ||
| suffix = '. Please make sure values that contains comma(,) are properly quoted.'; |
There was a problem hiding this comment.
| suffix = '. Please make sure values that contains comma(,) are properly quoted.'; | |
| suffix = '. Please make sure values that contain commas are properly quoted.'; |
| @@ -781,6 +781,7 @@ function addEdges( | |||
| } | |||
| } | |||
|
|
|||
| const FULLWIDTH_AMPERSAND = '＆'; // U+FF06, use as alt display for & so vis-network won't error out | |||
There was a problem hiding this comment.
So odd that this is even a thing.
| } | ||
|
|
||
| try { | ||
| new RegExp(nodeLabel); |
There was a problem hiding this comment.
Probably not a huge deal, but since regular expressions and exception throwing are relatively expensive, perhaps it would be more efficient to always do the replaceAll. Or are there cases where this substitution isn't a good thing?
There was a problem hiding this comment.
I was hoping to preserve the original labels as much as possible. The hope is that there aren't many that contains & to trigger this code.
There was a problem hiding this comment.
Probably true that there aren't many &-names, or we would have heard more about this problem, and maybe the world of JavaScript is different in this regard than Java, but I still think it's more efficient (and targeted) to do the replacement without regex construction, especially since it seems like this RegExp construction could throw an error for other reasons than the &.
There was a problem hiding this comment.
Sure that makes sense. Since user can't copy label from the vis canvas, it's no harm to do the replacement more aggressively. Updated to use replaceAll instead.
Rationale
Related Pull Requests
Changes
&with a similar unicode character for lineage graph labels display as vis-network doesn't work well with&