Skip to content
This repository has been archived by the owner on May 22, 2020. It is now read-only.

RUN-5538 - Expose Find In Page #987

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

luiemilio
Copy link
Member

@luiemilio luiemilio commented Oct 8, 2019

Description of Change

This exposes Electron's Find In Page API.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • automated tests are changed or added
  • relevant documentation is changed or added
  • PR title starts with the JIRA ticket pull request process
  • PR release notes describe the change in a way relevant to app-developers
  • Link to new tests added
  • PR has assigned reviewers

TEST

Release Notes

Notes:

Added API to find and highlight text on a page.

@openfin-github-bot openfin-github-bot bot added the auto testing started Bot started automated testing label Oct 8, 2019
@openfin-github-bot
Copy link

1b9405e

Git

  • core: develop <= RUN-5538_findInPage (1b9405e)
  • js-adapter: develop
  • javascript-adapter: develop

Asars used for testing

Test results

@openfin-github-bot openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 8, 2019
@@ -1110,6 +1110,11 @@ Window.executeJavascript = function(identity, code, callback = () => {}) {
WebContents.executeJavascript(browserWindow.webContents, code, callback);
};

Window.findInPage = function(identity, searchTerm, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually no, removing them now

@openfin-github-bot openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 8, 2019
@openfin-github-bot
Copy link

a15ac8a

Git

  • core: develop <= RUN-5538_findInPage (a15ac8a)
  • js-adapter: develop
  • javascript-adapter: develop

Asars used for testing

Test results

@openfin-github-bot openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 8, 2019
@openfin-github-bot openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 8, 2019
@openfin-github-bot
Copy link

c41ef02

Git

  • core: develop <= RUN-5538_findInPage (c41ef02)
  • js-adapter: develop
  • javascript-adapter: develop

Asars used for testing

Test results

@openfin-github-bot openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 8, 2019
@pbaize
Copy link
Contributor

pbaize commented Oct 9, 2019

Docs?

@luiemilio
Copy link
Member Author

Docs have been added to js-adapter

aziz512
aziz512 previously approved these changes Oct 11, 2019
Copy link
Contributor

@aziz512 aziz512 left a comment

Choose a reason for hiding this comment

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

Looks great!

const { payload } = message;
const { searchTerm, options } = payload;
const dataAck = Object.assign({}, successAck);
const windowIdentity = getTargetWindowIdentity(payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is there a reason we wouldn't want to use the identity here instead of the payload identity? Probably doesn't make a functional difference tho

Copy link
Member Author

Choose a reason for hiding this comment

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

All the other functions are using the payload one, didn't see a need to change that.

Copy link
Member

Choose a reason for hiding this comment

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

Identity (First param) is the requesting identity, the identity in the payout is the target.

@rdepena
Copy link
Member

rdepena commented Oct 16, 2019

Will a findInPage action generate a single event or multiple?

@luiemilio
Copy link
Member Author

findInPage now returns a promise with the request results generated by the 'found-in-page' event. I've also left the event in as it could be useful as well.

@openfin-github-bot openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 16, 2019
@openfin-github-bot
Copy link

780d860

Git

  • core: develop <= RUN-5538_findInPage (780d860)
  • js-adapter: develop
  • javascript-adapter: develop

Asars used for testing

Test results

@openfin-github-bot openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto testing done Bot completed automated testing cla-present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants