Fixes #198566: ellipsize left-cut search preview #198567
Conversation
|
|
||
| before = lcut(before, 26); | ||
| before = before.trimStart(); |
There was a problem hiding this comment.
We still need trimStart() for cases where the string's length is less than 26.
There was a problem hiding this comment.
@andreamah yes, quite right, I missed that.
However, looking at this, I've improved my understanding of lcut() - it seems that n is not a maximum but a target, given a string that's longer than n:
- with whitespace characters before, it can return less than
ncharacters, - but given non-whitespace characters, it can return more. The original tests show that with
assert.strictEqual(strings.lcut('foo bar', 5), 'foo bar');
I'm adding new tests that show when lcut() can return more than n characters, which is behaviour not affected by my PR.
Therefore, it feels that lcut() could also trim the start of a string that's already shorter. I've implemented your comment that way.
There was a problem hiding this comment.
I still see a case where an ellipsis is present where it shouldn't be. See below:
I'm assuming that the ... represents the leading whitespace, which is inconsistent with other cases where we trim whitespace and don't have a ....
There was a problem hiding this comment.
Thank you again, yes, this wasn't handled well. I've committed a fix. The code feels cleaner now, too.
jacekkopecky
force-pushed
the
bugfix/ellipsize-left-cut-search-preview
branch
from
780a2a5 to
833ac60
Compare
There was a problem hiding this comment.
LGTM! Thanks so much for contributing this!
Thinking about this change, I do wonder if people will disagree with it since it takes up space in the search preview that would otherwise not be present. However, we can discuss this more if it actually comes up. For now, we can try out this change and see what the reception is from it. 🙂
| after = this._oneLinePreviewText.substring(this._rangeInPreviewText.endColumn - 1); | ||
| preview(): { before: string; fullBefore: string; inside: string; after: string } { | ||
| const fullBefore = this._oneLinePreviewText.substring(0, this._rangeInPreviewText.startColumn - 1), | ||
| before = lcut(fullBefore, 26, '…'); |
There was a problem hiding this comment.
I won't block on this... but I wonder why this is done here and not in CSS using text-overflow.
There was a problem hiding this comment.
As a short answer, the reason it's not like that is because it was easier to modify the existing code to add an ellipsis.
Taking a quick skim of the spec for text-overflow, it seems that it would be based on an offset based on px whereas we seem to want to measure the offset by character. I might consider changing it to use text-overflow in the future, but it's probably not in the scope of this particular PR.
There was a problem hiding this comment.
text-overflow is also for the end of the string; we could abuse RTL text direction to cut from the start but then the ellipsis wouldn't reliably start at the start of the line.
This fixes #198566
The second screenshot in the issue is done with this PR in place.
Comments:
strings.ts, perhapslcut()should have the prefix default to '…' – it seems that the search preview generation is the only user of this function, and it would be consistent withtruncate().searchModel.ts, we don't need to dotrimStart()onbeforebecauselcut()already does that (and already did it before this PR, too).