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

repl: preserve preview on ESCAPE key press #46878

Merged
merged 3 commits into from
Mar 16, 2023
Merged

repl: preserve preview on ESCAPE key press #46878

merged 3 commits into from
Mar 16, 2023

Conversation

meixg
Copy link
Member

@meixg meixg commented Feb 28, 2023 •
edited by aduh95
Loading

Fixes: #46876

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Feb 28, 2023
@meixg meixg force-pushed the repl-escape-preview branch from 21dc2b9 to 61416f4 Compare February 28, 2023 10:08
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@meixg meixg force-pushed the repl-escape-preview branch from 61416f4 to d636ce8 Compare February 28, 2023 10:16
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50092/

BridgeAR
BridgeAR reviewed Feb 28, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

This seems like a nice improvement!
I would just not negate the argument and use an if statement.

lib/internal/repl/utils.js Outdated Show resolved Hide resolved
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
BridgeAR
BridgeAR approved these changes Feb 28, 2023
View reviewed changes
lib/internal/repl/utils.js Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
BridgeAR
BridgeAR reviewed Feb 28, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

I just noticed that this might require a bit more work: in case the line completion and the preview is active, it should remove both.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 28, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50094/

@meixg
Copy link
Member Author

meixg commented Feb 28, 2023 •
edited
Loading

I just noticed that this might require a bit more work: in case the line completion and the preview is active, it should remove both.

Do you mean:

> let someStr = '123'
undefined
> '0' + someStr // <- '0' + som
'0123'

then press escape ? I tried and it will remove both.

> let someStr = '123'
undefined
> '0' + som

@BridgeAR
Copy link
Member

@meixg yes, that's what I meant. I just saw that it will work as expected due to resetting the preview suffix to null and therefore not having a match anymore. Good work!

@github-actions github-actions bot mentioned this pull request Mar 1, 2023
Open
15 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50116/

This was referenced Mar 2, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50177/

@meixg meixg force-pushed the repl-escape-preview branch from 7eef4d7 to 24cec13 Compare March 3, 2023 03:26
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 3, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50178/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50209/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50218/

@meixg meixg force-pushed the repl-escape-preview branch from 24cec13 to 3e54f3a Compare March 10, 2023 11:36
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2023
@meixg meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 11, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50317/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50321/

This was referenced Mar 12, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50351/

@github-actions github-actions bot mentioned this pull request Mar 14, 2023
Open
24 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50374/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/50378/

BridgeAR
BridgeAR approved these changes Mar 14, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Still LGTM

This was referenced Mar 15, 2023
Open
Open
@meixg meixg added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 16, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 16, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2566400 into nodejs:main Mar 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2566400

This was referenced Mar 17, 2023
Open
Open
targos pushed a commit that referenced this pull request Mar 18, 2023
@meixg @targos
repl: preserve preview on ESCAPE key press
d6a1232 
Fix: #46876
PR-URL: #46878
Fixes: #46876
Reviewed-By: Ruben Bridgewater <[email protected]>
This was referenced Apr 3, 2023
Merged
Merged
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@meixg @danielleadams
repl: preserve preview on ESCAPE key press
e668efa 
Fix: #46876
PR-URL: #46878
Fixes: #46876
Reviewed-By: Ruben Bridgewater <[email protected]>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

repl: preview removed when press ESC key
3 participants
@meixg @nodejs-github-bot @BridgeAR