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

[Editor] Unselect correctly removed editors #15210

Merged
merged 1 commit into from
Jul 22, 2022

Conversation

calixteman
Copy link
Contributor

  • After undoing a deletion of several editors, they appeared to be selected (they had a red border)
    when in fact they were not, consequently, this patch aims to remove the selectedEditor class when
    an editor is removed;
  • Add a test with some ink editors.

exports.editorPrefix = editorPrefix;

function getSelectedEditors(page) {
return page.evaluate(pref => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Can this helper function perhaps use the same parameter name as before it was moved to this file, since I find "prefix" a slightly clearer name overall (since "pref" could generally be short for e.g. preference as well)?

Suggested change
return page.evaluate(pref => {
return page.evaluate(prefix => {

Comment on lines 84 to 85
for (const element of elements) {
results.push(parseInt(element.id.slice(pref.length)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: This could maybe be shortened a tiny bit?

Suggested change
for (const element of elements) {
results.push(parseInt(element.id.slice(pref.length)));
for (const { id } of elements) {
results.push(parseInt(id.slice(pref.length)));

- After undoing a deletion of several editors, they appeared to be selected (they had a red border)
when in fact they were not, consequently, this patch aims to remove the selectedEditor class when
an editor is removed;
- Add a test with some ink editors.
@calixteman
Copy link
Contributor Author

/botio integrationtest

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/0b79c0d295b1e1a/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/feec9d972aebf9e/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/feec9d972aebf9e/output.txt

Total script time: 4.82 mins

  • Integration Tests: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/0b79c0d295b1e1a/output.txt

Total script time: 7.52 mins

  • Integration Tests: FAILED

@calixteman calixteman merged commit 185102b into mozilla:master Jul 22, 2022
@calixteman calixteman deleted the ink_select branch July 22, 2022 11:31
@nmtigor nmtigor mentioned this pull request Sep 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants