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

feat(view): introduce LayoutChanged event on every View component #5825

Merged
merged 5 commits into from
May 21, 2018

Conversation

ADjenkov
Copy link
Contributor

LayoutChanged event should be invoked when the layout bounds of a view changes.

Fix #5687

@@ -305,6 +307,17 @@ export class View extends ViewCommon {
public initNativeView(): void {
super.initNativeView();
this._isClickable = this.nativeViewProtected.isClickable();

this.setOnLayoutChangeListener();
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create a LayoutChangeListener only if someone has attached to this event. Currently listeners will be created for all views and will be triggered unnecessary.
We are doing something similar with the touch listener here. For the onLayout we should probably override another method as it is not attached trough observe() (observe is for gestures). Probably on()/off() events

@ADjenkov ADjenkov force-pushed the djenkov/layout-changed-event branch from ebdabf2 to 2e45de7 Compare May 17, 2018 13:57
newPage.content = stackLayout;

TKUnit.waitUntilReady(() => stackLayoutChanged && buttonLayoutChanged);
TKUnit.assert(stackLayoutChanged);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure we need those explicit asserts -- waitUnitlReady(...) on previous line will time out if those booleans are not true.

Copy link
Contributor Author

@ADjenkov ADjenkov May 18, 2018

Choose a reason for hiding this comment

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

I'm not sure too.
We do it in other tests

@manoldonev
Copy link
Contributor

#5687 says that the main use case is to be able to use getActualSize() and be sure values are correct -- shouldn't we add a test for this?

@@ -241,6 +243,26 @@ export class View extends ViewCommon {
}
}

on(eventNames: string, callback: (data: EventData) => void, thisArg?: any) {
super.on(eventNames, callback, thisArg);
const isLayoutEvent = typeof eventNames === "string" ? eventNames.indexOf(ViewCommon.layoutChangedEvent) !== -1 : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

"string" ? -- two spaces instead of one.

@@ -305,6 +327,19 @@ export class View extends ViewCommon {
public initNativeView(): void {
super.initNativeView();
this._isClickable = this.nativeViewProtected.isClickable();

if (this.hasListeners(ViewCommon.layoutChangedEvent)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to check for !this.layoutChangeListenerIsSet here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should not be possible to initNativeView twice without disposeNativeView

@ADjenkov ADjenkov force-pushed the djenkov/layout-changed-event branch from dd9f0b5 to dc1b36b Compare May 21, 2018 07:46
@ADjenkov
Copy link
Contributor Author

test

1 similar comment
@ADjenkov
Copy link
Contributor Author

test

@ADjenkov ADjenkov dismissed vakrilov’s stale review May 21, 2018 14:21

I've talked with the runtime guys and the way we use the listener is ok

@ADjenkov ADjenkov merged commit 0fc1547 into master May 21, 2018
@ADjenkov ADjenkov deleted the djenkov/layout-changed-event branch May 21, 2018 14:26
@lock
Copy link

lock bot commented Aug 26, 2019

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.

@lock lock bot locked and limited conversation to collaborators Aug 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Expose onLayout event on View
3 participants