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

Window refresh 2 named exports #986

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

Conversation

pbaize
Copy link
Contributor

@pbaize pbaize commented Oct 7, 2019

Description of Change

Switch to named exports from window.js

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • PR has assigned reviewers

Release Notes

Notes: n/a internal

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

bc19553

Git

  • core: develop <= window-refresh-2 (bc19553)
  • 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 7, 2019
@@ -21,7 +21,7 @@ let minimist = require('minimist');
// local modules
let Application = require('./src/browser/api/application.js').Application;
let System = require('./src/browser/api/system.js').System;
import { Window } from './src/browser/api/window';
import { create } from './src/browser/api/window';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Considering this file is as huge as it is, I think it's more descriptive to have the Window.create call vs just a plain create call. Can be hard to tell exactly what's being created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying not to import the whole file here, it helps with circular dependencies on the typescript conversion

Copy link
Contributor

@MichaelMCoates MichaelMCoates left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants