LG-4798: code custom snips obj #2728
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… into LG-4760/code-copy-button
🦋 Changeset detectedLatest commit: 9fa19d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +446 B (+0.03%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
tsck
reviewed
Feb 20, 2025
Comment on lines
172
to
180
| if (typeof child === 'string') { | ||
| if (typeof lastItem === 'string') { | ||
| acc[acc.length - 1] = lastItem + child; | ||
| } else { | ||
| acc.push(child); | ||
| } | ||
| } else { | ||
| acc.push(child); | ||
| } |
There was a problem hiding this comment.
I think this can be simplified slightly:
Suggested change
| if (typeof child === 'string') { | |
| if (typeof lastItem === 'string') { | |
| acc[acc.length - 1] = lastItem + child; | |
| } else { | |
| acc.push(child); | |
| } | |
| } else { | |
| acc.push(child); | |
| } | |
| if (typeof child === 'string' && typeof lastItem === 'string') { | |
| acc[acc.length - 1] = lastItem + child; | |
| } else { | |
| acc.push(child); | |
| } |
| return function ( | ||
| entity: string | TokenObject, | ||
| ): string | FlatTokenObject | Array<string | FlatTokenObject> { | ||
| // console.log({ entity }); |
There was a problem hiding this comment.
Accidentally left behind?
Suggested change
| // console.log({ entity }); |
| * ``` | ||
| */ | ||
| const lineWithKeywords = (line: Array<string | FlatTokenObject>) => { | ||
| const mergedLines = mergeStringsIntoString(line); |
There was a problem hiding this comment.
Q: What benefit do we get from merging the words together? Is there a functional reason this is required?
There was a problem hiding this comment.
Yeah, say, for example, you want to highlight _hello, I'm accounting for cases where the plugin might return this as:
['_', 'hello ', 'world ', 'hi', {}, 'bye ', '_', 'hello ']Merging this into one string makes it easer to find _hello. The output of merging would be:
['_hello world hi', {}, 'bye _hello']So this is a just-in-case thing.
| * ['_hello world hi', {}, 'bye _hello'] => [{ kind: 'lg-highlight-custom', children: ['_hello'] }, 'world ', 'hi', {}, 'bye', { kind: 'lg-highlight-custom', children: ['_hello'] }] | ||
| * ``` | ||
| */ | ||
| const lineWithKeywords = (line: Array<string | FlatTokenObject>) => { |
There was a problem hiding this comment.
Q: Do we / could we add some tests around this logic?
| * | ||
| * E.g. `customKeywordObject: {{ 'keyword': 'className' }}` | ||
| */ | ||
| customKeywordObject?: Record<string, string>; |
There was a problem hiding this comment.
Nit: I'm on the fence about having Object in this prop name. I think I'd prefer just customKeywords or customKeywordConfig. Not a hard stance and definitely not blocking, but curious to hear your thoughts.
There was a problem hiding this comment.
I don't mind removing Object.
|
Should this be a minor bump version bump? Or are we only bumping the version on the integration branch? |
I think I'll keep this independent from the integration branch and merge it after the integration branch merges. |
tsck
approved these changes
Feb 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
🎟 Jira ticket: LG-4798
We had a request to change the color of certain words in the code editor. In particular, this ticket wanted to highlight
passwordandusername.Some examples of this include:
I initially tried an approach of wrapping the word to highlight in a sequence of characters:
However, this approach was challenging because we support a wide range of languages, making it difficult to find a character sequence that wouldn’t interfere with language-specific special characters.
I decided to use an object-based approach. Consumers can pass in an object containing multiple keywords they want to customize. I'll search for each keyword and wrap it in a div with the specified className. However, if the keyword is a reserved word in that language, it will be ignored. The only drawback to this approach is that it will target every occurrence of the keyword.
✅ Checklist
For bug fixes, new features & breaking changes
pnpm changesetand documented my changesFor new components
🧪 How to test changes