test

@SvetoslavTsenov what does that mean? Also I can't see the details log of ci/jenkins/core-modules-tests — FAILed. So I can't see what to fix

Hey @farfromrefug, I have executed some integration tests on our CI (which is internal). The result so far is that unit tests are failing for both platform and uitests app crashes on start up(you can find it in app folder). Could you please rebase your branch again and run unit tests on you side. To do that you can simple do:
npm run setup npm run tsc cd tests tns run ios/ android

Let me know if you need some further information.

@SvetoslavTsenov I realised the issue now. On iOS createNativeView is not used.
I also realise that there is a comment about this in the code saying it should be done the same way as in android and use it.

DO you want me to expand that PR to do that? I am willing to but it would be more changes for iOS.
Could make that PR harder to merge.
I
I still defined createNativeView for iOS Label and Button but call It in the constructor. Also I only create if not already created.
This does not follow the same rule as Android. But otherwise tests will fail.

@SvetoslavTsenov still can't see what tests are failing with ci/jenkins/core-modules-tests — FAILed.. Details page brings fail to open page

As for the tests app. It succeeds now on iOS but I keep getting waitUntilReady Timeout., Stack: Error: waitUntilReady Timeout. errors on android. Any idea?
Finally I have a lot of tests failing on android not related to what I have done. Like :
Test: --- [ACTION-BAR.test_ActionBar_set_title_as_number_doesnt_thrown] FAILED: Expected: false, Actual: true, Stack: Error: Expected: false, Actual: true

So not sure how to know what makes the cli fail

Hi @farfromrefug, thanks for the PR and thumbs up for the material components plugin. Just a heads up - the repo you linked to the plugin appears to be private, so we can't take a look.

I reviewed the PR, discussed it with the team and we agree with the changes.

• Just for context and reference, I think you should take a look at ViewBase's setupUI method. This is the place where createNativeView() and initNativeView() are called and where the backwards compatibility for native view create in constructor is handled.
• Also, if you haven't already, take a look at this article in the docs - https://docs.nativescript.org/plugins/ui-plugin-custom#building-ui-plugins-using-custom-components - largely explaining what you are trying to do.
• We consider the iOS components native view create in constructor as technical debt and we should move to createNativeView() on all components eventually. The goal is to have a unified api with the Android components.

That being said, here are some comments on how to improve the PR:

• On all the iOS components where you added createNativeView() remove the constructor altogether. The createNativeView() method will be called from the setupUI() method.
• There is no need to couple the new createDelegate(nativeView) method with createNativeView(). I think you even forgot to call it in editable-text-base.android.ts. My suggestion is to call this method in setupUI() right after createNativeView(). Of course this would mean to add it in view-base.d.ts and view-base.ts with an empty comment for backwards compatibility.
• Also, please add a comment to the nativeTextViewProtected getter on why we are doing this and why you would want to override it.

@MartoYankov Thanks.
Sorry about the link. I fixed it:
https://github.com/Akylas/nativescript-material-components

@MartoYankov great comments.

I will look at all that !

Thanks

@MartoYankov Made changes based on your comments.
There is one thing bugging still. In text-field.ios.ts we now create to weak references to this because createNativeView and initNativeViewDelegates are not called in the same place anymore. Is that a big deal?

Should I update all iOS classes or do we do that in another work package?

@farfromrefug I think the two weak references to this are fine and also, updating the other iOS and Android classes can happen in separate PRs, so that they are more manageable.

Now, seeing that implemented I have some doubts about the new initNativeViewDelegates method that has a parameter any. It's a bit confusing having both initNativeViewDelegates and initNativeView. What do you think about moving the contents of the initNativeViewDelegates to initNativeView? Will it work for your case? It will have the added benefit that during initNativeView we already have nativeViewProtected, so you won't have to pass parameters.

I'm thinking about making a PR to your fork if you are okay with the change. Let me know what you think.

@MartoYankov a PR is fine !
Now about moving the initNativeViewDelegates inside initNativeView That s actually what I try to get around.
The issue is that if you component uses a subclass of the nativeView Nativescript is using you want to have a custom initNativeView but keep the delegate from Nativescript core.
The real issue is that otherwise you have to declare again all the "listener" classes like the ones define here

@farfromrefug That way you would avoid repeating the listeners, but there is still some custom code in initNativeView like assigning the listener owner. Wouldn't it be better if in your inheritor class you call the super.initNativeView() which would initialize the listeners of the base NativeScript core class and then put your custom initNativeView logic there?

@MartoYankov sorry I think I misplaced initNativeView and createNativeView ...
So I am ok with initNativeView being the place where all listener/delegates are created.
One thing: it seems initNativeView is never used on iOS. No issue there?
Cause I would move all delegate creation there even on iOS.

Also isn't initNativeView called multiple times? I mean it's called from setNativeView which is called from nativeView setter which could be called often? And there is no check to see if new nativeView is equal to current one. So if called again with same view, initNativeView will be called again and thus all listener created again.

BTW I am thinking about upgrading all ui components from that PR. Any issue with that?

@farfromrefug Regarding your first question - yes, the current state of affairs is that iOS components are using their constructors to create and init, while Android components are using createNativeView and initNativeView. As I previously commented, we consider this technical debt and should move the code from all iOS constructors to createNativeView and initNativeView.

Regarding your second question - the nativeView setter shouldn't be called often. I'm not sure about the scenario where it was needed, but there is a check if the new value is equal to the current one. The first check in setNativeView should account for that.

I'm okay with making the changes to all components in this PR. We have to change the title though.

test

test

test

@farfromrefug Some dialogs and modals UI tests are failing. I know you can't see them, so I'll take care of them.

@MartoYankov ok let me know If I can help in any way

test

test --ignore branch_ns_ui_sidedrawer_demo ios11 api23

@farfromrefug Thanks for the awesome PR. I hope the material components will come out nicely now.

@MartoYankov Thanks! Yes will update my plugin right now with this. Need some changes on the runtime though (might already be in right now) for that plugin to be published.

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.