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

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
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