Feat/Text highlight #1514
Feat/Text highlight #1514
Conversation
| } | ||
| const parts = []; | ||
| let val = false; | ||
| for (let k = 0; k < indices.length; k++) { |
There was a problem hiding this comment.
What about using a forEach or even a map in this case?
There was a problem hiding this comment.
I think I can't use map or forEach here since in every iteration I use the current index and the next one (k and k+1) which is not possible with those methods..
There was a problem hiding this comment.
Yes, map and forEach that comes with javascript don't retrieve the Index..
But using lodash's _.forEach or _.map, you also have access to the index
_.map(array, (item, index) => console.log(index))
_.forEach(array, (item, index) => console.log(index))
There was a problem hiding this comment.
I see, but I need the item and the next item..
I can use it that way:
_.forEach(array, (item, index) => console.log(item, array[index + 1]))
but I don't think it makes that more clear..
There was a problem hiding this comment.
Ok, I have one last suggestion, let me know what you think (:
What if instead of keeping the indices as couples (which I personally find confusing) you'll keep them as objects of {start, end}, so indices looks like this
[{start: 2, end: 6}, {start: 10, end: 12}, ...]
I think it'll make a easier to understand (:
WDYT?
src/components/text/index.tsx
Outdated
| if (i >= 0) { | ||
| const newIndex = index + lastWordLength + i; | ||
| indices.push(index + lastWordLength + i); | ||
| indices.push(index + lastWordLength + i + word.length); | ||
| index = newIndex; | ||
| lastWordLength = word.length; | ||
| } else { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
I have a suggestion that can shorten this code a little (:
Let me know if it can work..
First, regardless.. lodash has lots of great utils I'd use instead of for..loops when possible, I find them easier to read.
Anyway, there's the _.split util.
You can go over the target string and split it according to the highlight strings array and this way break the strings as you did without having to save their indices.
Here I had to understand you saved the indices in couples of start and end of highlight and it's not really clear at first glance.
There was a problem hiding this comment.
I can't find a way to make it work with the split method.
If I don't keep the indices I can only return a lower case string and not the original one...
There was a problem hiding this comment.
Yes, I understand.
It's just I'd like to make the code a little more readable.
(Btw, my old code there is not so readable also)
Everything together makes the Text component to look a bit too much so I was trying to think of ways to clean this up a little before it all become legacy code.
...Hmm another thought of how to shorten this up.
Do you think we can take the use case of a single highlight string and use your implementation as well?
(and then we can remove my old implementation)
Technically if the user pass a string (and not an array of strings) we can just pass it to your method wrapped with an array.
WDYT?
There was a problem hiding this comment.
Actually, we said that if the user passes only one string we highlight all the repeats of this string (as the old method does), and if he passes an array of strings we highlight it according to its order (as the new method does)
There was a problem hiding this comment.
Oh right, I remember there was a good reason for the separation.
Description
Update Text highlight method
WOAUILIB-2058
Changelog
Update Text highlight method