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

[TIMOB-25522] iOS: Add "navigationWindow" property to Ti.UI.Window, add unit-tests for all nav-window API's #9608

Merged
merged 31 commits into from May 21, 2018

Conversation

hansemannn
Copy link
Collaborator

win = null;
});

it.ios('#navigationWindow', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been trying to use a naming scheme so that #whatever is used to denote methods, .whatever is used to denote properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gotcha, will change!

finish();
});

var nav = Ti.UI.iOS.createNavigationWindow({
Copy link
Contributor

Choose a reason for hiding this comment

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

var declaration should always be at the top of the scope (or else eslint will complain, since vars are hoisted anyways)
So var nav; should go at the very top, then the assignment stays here.

var should = require('./utilities/assertions'),
utilities = require('./utilities/utilities');

describe('Titanium.UI.Window', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename this file to ti.ui.window.navigationwindow.addontest.js so it gets "added on" the existing suite rather than replaces the existing ti.ui.window tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively, you could add a ti.ui.ios.navigationwindow.addontest.js for this create/property test and any future tests on that type (I see we have 3 methods that likely should have tests: http://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI.iOS.NavigationWindow)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How to deal with before, after, beforeEach and afterEach methods? Should I include those in the addons?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like to be safe on Android the before/after should stay. You don't check for, so don't need didFocus or before hook. I'd keep the after hook, and drop your explicit setting of win = null in the test (is that needed?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, will do. Copied over the win = null statement, don't think it's needed as the proxy should clean up after itself.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Decided to use ti.ui.ios.navigationwindow.addontest.js and will add unit-tests for the other ones as well to get some more basic coverage. And as Android / Windows will get parity on the navigation-window, they'll be happy to find existing unit-tests already.

@hansemannn hansemannn changed the title [TIMOB-25522] iOS: Add "navigationWindow" property to Ti.UI.Window [TIMOB-25522] iOS: Add "navigationWindow" property to Ti.UI.Window, add unit-tests for all nav-window API's Nov 29, 2017
@@ -41,6 +41,72 @@ describe('Titanium.UI.Window', function () {
}
win = null;
});

it.ios('#openWindow', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, you can just do describe.ios at the top to limit all sub-tests to iOS and avoid having to do it.ios on them all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doing now, thx!

@hansemannn hansemannn added this to the 7.2.0 milestone Feb 16, 2018
@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@ssjsamir ssjsamir self-requested a review May 17, 2018 15:27
Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Able to reference the current navigation-window using the "navigationWindow" instance property. From the following test case from https://jira.appcelerator.org/browse/TIMOB-25522:

var win = Ti.UI.createWindow({
  backgroundColor: '#fff'
});
 
var nav = Ti.UI.iOS.createNavigationWindow({
  window: win
});
 
var btn = Ti.UI.createButton({
  title: 'Open next window'
});
 
btn.addEventListener('click', function() {
  win.navigationWindow.openWindow(Ti.UI.createWindow({
    title: 'Next Window!',
    backgroundColor: 'green'
  }));
});
 
win.add(btn);
nav.open();

Test steps

  • Created a build from this PR
  • Created a titanium app
  • Added the test case above
  • Pressed 'open newt window'
  • New window was opened

Test Environment
APPC Studio: 5.0.0.201712081732
APPC CLI: 7.0.3
iphone 6 plus (10.2)
Operating System Name: Mac OS High Sierra
Operating System Version: 10.13
Node.js Version: 8.9.1
Xcode 9.2

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy I cannot figure out why the unit tests here are still failing.

@build
Copy link
Contributor

build commented May 18, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn hansemannn merged commit 2f34df7 into tidev:master May 21, 2018
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.

None yet

5 participants