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.

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

Select all patch entities or input contents from menu item #1505

Merged
merged 3 commits into from
Oct 30, 2018

Conversation

brusherru
Copy link
Contributor

It fixes #1497.

Also, I've added "Select All" menu item into browser version of the IDE, that selects all entities on the Patch.

@brusherru brusherru self-assigned this Oct 24, 2018
@brusherru brusherru requested a review from a team October 24, 2018 17:52
Copy link
Contributor

@evgenykochetkov evgenykochetkov left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -575,7 +575,19 @@ class App extends client.App {
items.cut,
items.copy,
items.paste,
items.selectall,
onClick(items.selectall, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Too complex function for the purely structural code span. Suggest extracting it to a method

@@ -575,7 +575,19 @@ class App extends client.App {
items.cut,
items.copy,
items.paste,
items.selectall,
onClick(items.selectall, () => {
// We can't use `role: 'selectall'` here, cause it ignores `onClick`.
Copy link
Member

Choose a reason for hiding this comment

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

📜 because

@brusherru
Copy link
Contributor Author

@nkrkv check it out, please

@nkrkv nkrkv added the wip label Oct 29, 2018
@brusherru brusherru force-pushed the fix-1497-select-all branch 2 times, most recently from 50de875 to 1d44f5f Compare October 29, 2018 09:28
@brusherru brusherru removed the wip label Oct 29, 2018
@brusherru
Copy link
Contributor Author

@nkrkv check it out, please

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

The code looks excellent now 👍

However, it does not work for me 😞 On the desktop version, Ctrl+A selects contents of Quick Help whatever I click. The same for hitting Edit → Select All

@brusherru
Copy link
Contributor Author

@nkrkv Oops. My bad.
Moved function into App container method, but forget to bind it 🙈
Fixed!

@brusherru
Copy link
Contributor Author

brusherru commented Oct 29, 2018

@nkrkv finally, it has to work.
Tested on Ubuntu in VirtualBox.

Copy link
Member

@nkrkv nkrkv left a comment

Choose a reason for hiding this comment

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

Whew 😌 Now it works on my platform (tested both browser and electron). Please, fix wording, and merge

/**
* We have to handle some hotkeys, because:
* - Browser IDE should prevent default event handling
* - Electron IDE could not handle correct some hotkeys.
Copy link
Member

Choose a reason for hiding this comment

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

📜 Electron IDE cannot handle some hotkeys correctly

@brusherru brusherru merged commit edaceb0 into 0.25.x Oct 30, 2018
@brusherru brusherru deleted the fix-1497-select-all branch October 30, 2018 14:53
@nkrkv nkrkv added the hotfix Issue/PR for a patch release label Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotfix Issue/PR for a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants