Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace substr with substring #7188

Merged
merged 3 commits into from
Feb 18, 2025
Merged

Replace substr with substring #7188

merged 3 commits into from
Feb 18, 2025
+5 −5

Conversation

ivailop7
Copy link
Collaborator

substr is deprecated, replacing with the current equivalent to silence the linters

@vercel Vercel
Copy link

vercel bot commented Feb 16, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 17, 2025 11:34pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 17, 2025 11:34pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 16, 2025
@ivailop7 ivailop7 added extended-tests Run extended e2e tests on a PR and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 16, 2025
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 16, 2025
etrepum
etrepum reviewed Feb 16, 2025
View reviewed changes
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of these things (is not like the others)

packages/lexical-react/src/shared/LexicalMenu.ts Outdated
@@ -111,7 +111,7 @@ function getFullMatchOffset(
): number {
let triggerOffset = offset;
for (let i = triggerOffset; i <= entryText.length; i++) {
if (documentText.substr(-i) === entryText.substr(0, i)) {
if (documentText.substring(-i) === entryText.substring(0, i)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not equivalent, substr(startIndex, length) and substring(startIndex, endIndex) don't work the same for substr(-i) and substring(-i). Probably should be something like documentText.substring(documentText.length - i) instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Will fix this evening.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just occurred to me that there's also the slice method, documentText.slice(-i) is shorter and does the same thing

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substring#differences_between_substring_and_slice

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'slice' shout out is a good one. updated!

etrepum
etrepum approved these changes Feb 18, 2025
View reviewed changes
Copy link
Collaborator

@etrepum etrepum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could've been slice all around to save a few bytes, but this works just the same!

@ivailop7 ivailop7 added this pull request to the merge queue Feb 18, 2025
Merged via the queue into facebook:main with commit 4656ac1 Feb 18, 2025
39 checks passed
@ivailop7
Copy link
Collaborator Author

LGTM, could've been slice all around to save a few bytes, but this works just the same!

Should probably reuse the utils method in all other plugins in playground and should probably save more bytes, if minifaction doesn't pick those up already behind the scenes

@ivailop7 ivailop7 deleted the replace_substr branch February 18, 2025 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etrepum etrepum etrepum approved these changes

@zurfyx zurfyx Awaiting requested review from zurfyx zurfyx is a code owner

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

@Fetz Fetz Awaiting requested review from Fetz Fetz is a code owner

@Sahejkm Sahejkm Awaiting requested review from Sahejkm Sahejkm is a code owner

@potatowagon potatowagon Awaiting requested review from potatowagon potatowagon is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivailop7 @etrepum @facebook-github-bot