Skip to content

Added get/setParent to ContainerWindow#308

Merged
bingenito merged 2 commits intomorganstanley:masterfrom
Sly1024:set-parent
May 7, 2020
Merged

Added get/setParent to ContainerWindow#308
bingenito merged 2 commits intomorganstanley:masterfrom
Sly1024:set-parent

Conversation

@Sly1024
Copy link
Copy Markdown
Contributor

@Sly1024 Sly1024 commented May 6, 2020

No description provided.

@Sly1024 Sly1024 requested a review from a team May 6, 2020 21:20
}

public getParent(): Promise<ContainerWindow> {
// on the web, there's no parent
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is opener a parent?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I found this PR (it's symphony, not openfin - or is it?) where they treat opener as parent.
However, in our case I don't think we should. The parent window:

  • is always behind the child windows
  • closes its children when closed
  • activates/resotres its children when restored

The opener doesn't do any of those.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we blank out the getParent you have in OpenFin here as I think that is the opener?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We don't really use getParent, and if they implement setParent correctly, they must "fix" getParent as well. However, to make setParent/getParent calls consistent, I think we should blank it out.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2020

Codecov Report

Merging #308 into master will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   93.37%   93.43%   +0.06%     
==========================================
  Files          16       16              
  Lines        1539     1554      +15     
  Branches      265      265              
==========================================
+ Hits         1437     1452      +15     
  Misses        102      102              
Impacted Files Coverage Δ
packages/desktopjs/src/window.ts 88.88% <ø> (ø)
packages/desktopjs-electron/src/electron.ts 97.10% <100.00%> (+0.04%) ⬆️
packages/desktopjs-openfin/src/openfin.ts 87.67% <100.00%> (+0.17%) ⬆️
packages/desktopjs/src/Default/default.ts 94.62% <100.00%> (+0.09%) ⬆️

psmulovics
psmulovics previously approved these changes May 6, 2020
Copy link
Copy Markdown
Member

@bingenito bingenito left a comment

Choose a reason for hiding this comment

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

lgtm

@bingenito bingenito merged commit 2588729 into morganstanley:master May 7, 2020
@Sly1024 Sly1024 deleted the set-parent branch May 7, 2020 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants