-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
fix(angular): inline overlay dynamic template data #24521
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
describe('Popovers: Inline', () => { | ||
beforeEach(() => { | ||
cy.visit('/popover-inline'); | ||
}); | ||
|
||
it('should initially have no items', () => { | ||
cy.get('ion-list ion-item').should('not.exist'); | ||
}); | ||
|
||
it('should have items after 1500ms', () => { | ||
cy.wait(1500); | ||
|
||
cy.get('ion-list ion-item:nth-child(1)').should('have.text', 'A'); | ||
cy.get('ion-list ion-item:nth-child(2)').should('have.text', 'B'); | ||
cy.get('ion-list ion-item:nth-child(3)').should('have.text', 'C'); | ||
cy.get('ion-list ion-item:nth-child(4)').should('have.text', 'D'); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export * from './modal-inline.component'; | ||
export * from './modal-inline.module'; |
16 changes: 16 additions & 0 deletions
16
angular/test/test-app/src/app/modal-inline/modal-inline-routing.module.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { NgModule } from "@angular/core"; | ||
import { RouterModule } from "@angular/router"; | ||
import { ModalInlineComponent } from "."; | ||
|
||
@NgModule({ | ||
imports: [ | ||
RouterModule.forChild([ | ||
{ | ||
path: '', | ||
component: ModalInlineComponent | ||
} | ||
]) | ||
], | ||
exports: [RouterModule] | ||
}) | ||
export class ModalInlineRoutingModule { } |
11 changes: 11 additions & 0 deletions
11
angular/test/test-app/src/app/modal-inline/modal-inline.component.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<ion-modal [isOpen]="true" [breakpoints]="[0.1, 0.5, 1]" [initialBreakpoint]="0.5"> | ||
<ng-template> | ||
<ion-content> | ||
<ion-list> | ||
<ion-item *ngFor="let item of items"> | ||
<ion-label>{{ item }}</ion-label> | ||
</ion-item> | ||
</ion-list> | ||
</ion-content> | ||
</ng-template> | ||
</ion-modal> |
21 changes: 21 additions & 0 deletions
21
angular/test/test-app/src/app/modal-inline/modal-inline.component.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { AfterViewInit, Component } from "@angular/core"; | ||
|
||
/** | ||
* Validates that inline modals will correctly display | ||
* dynamic contents that are updated after the modal is | ||
* display. | ||
*/ | ||
@Component({ | ||
selector: 'app-modal-inline', | ||
templateUrl: 'modal-inline.component.html' | ||
}) | ||
export class ModalInlineComponent implements AfterViewInit { | ||
|
||
items: string[] = []; | ||
|
||
ngAfterViewInit(): void { | ||
setTimeout(() => { | ||
this.items = ['A', 'B', 'C', 'D']; | ||
}, 1000); | ||
} | ||
} |
12 changes: 12 additions & 0 deletions
12
angular/test/test-app/src/app/modal-inline/modal-inline.module.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
import { CommonModule } from "@angular/common"; | ||
import { NgModule } from "@angular/core"; | ||
import { IonicModule } from "@ionic/angular"; | ||
import { ModalInlineRoutingModule } from "./modal-inline-routing.module"; | ||
import { ModalInlineComponent } from "./modal-inline.component"; | ||
|
||
@NgModule({ | ||
imports: [CommonModule, IonicModule, ModalInlineRoutingModule], | ||
declarations: [ModalInlineComponent], | ||
exports: [ModalInlineComponent] | ||
}) | ||
export class ModalInlineModule { } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from './popover-inline.module'; |
14 changes: 14 additions & 0 deletions
14
angular/test/test-app/src/app/popover-inline/popover-inline-routing.module.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { NgModule } from "@angular/core"; | ||
import { RouterModule } from "@angular/router"; | ||
import { PopoverInlineComponent } from "./popover-inline.component"; | ||
|
||
@NgModule({ | ||
imports: [RouterModule.forChild([ | ||
{ | ||
path: '', | ||
component: PopoverInlineComponent | ||
} | ||
])], | ||
exports: [RouterModule] | ||
}) | ||
export class PopoverInlineRoutingModule { } |
11 changes: 11 additions & 0 deletions
11
angular/test/test-app/src/app/popover-inline/popover-inline.component.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<ion-popover [isOpen]="true"> | ||
<ng-template> | ||
<ion-content> | ||
<ion-list> | ||
<ion-item *ngFor="let item of items"> | ||
<ion-label>{{ item }}</ion-label> | ||
</ion-item> | ||
</ion-list> | ||
</ion-content> | ||
</ng-template> | ||
</ion-popover> |
22 changes: 22 additions & 0 deletions
22
angular/test/test-app/src/app/popover-inline/popover-inline.component.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { AfterViewInit, Component } from "@angular/core"; | ||
|
||
/** | ||
* Validates that inline popovers will correctly display | ||
* dynamic contents that are updated after the modal is | ||
* display. | ||
*/ | ||
@Component({ | ||
selector: 'app-popover-inline', | ||
templateUrl: 'popover-inline.component.html' | ||
}) | ||
export class PopoverInlineComponent implements AfterViewInit { | ||
|
||
items: string[] = []; | ||
|
||
ngAfterViewInit(): void { | ||
setTimeout(() => { | ||
this.items = ['A', 'B', 'C', 'D']; | ||
}, 1000); | ||
} | ||
|
||
} |
11 changes: 11 additions & 0 deletions
11
angular/test/test-app/src/app/popover-inline/popover-inline.module.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import { CommonModule } from "@angular/common"; | ||
import { NgModule } from "@angular/core"; | ||
import { IonicModule } from "@ionic/angular"; | ||
import { PopoverInlineRoutingModule } from "./popover-inline-routing.module"; | ||
import { PopoverInlineComponent } from "./popover-inline.component"; | ||
|
||
@NgModule({ | ||
imports: [CommonModule, IonicModule, PopoverInlineRoutingModule], | ||
declarations: [PopoverInlineComponent], | ||
}) | ||
export class PopoverInlineModule { } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be done for inline popovers as well? https://github.com/ionic-team/ionic-framework/blob/main/angular/src/directives/overlays/popover.ts#L107
Additionally, how does this impact change detection with a controller (if it impacts it at all)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this did affect popovers as well. I've updated the PR to resolve and test both. I've also reworked the commit since this is more about inline overlays and less specific to just modals.
Tested with controllers & attached change detection and things are working as expected. I cannot notice an impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why this call was added in the first place, and traced it back to the inline modal feature PR: https://github.com/ionic-team/ionic-framework/pull/23341/files#diff-d123478d8d877b48ad22b0a74610bcd649b94548e5e5c56a173bb3506d506e88R25 @liamdebeasi Can you think of why this was added originally? I'm wondering if there was some edge case bug this was meant to fix 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.detach()
is what all the other components do, which is why I added it https://github.com/ionic-team/ionic-framework/blob/main/angular/src/directives/proxies.ts#L26There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh... do we have our own change detection set up or something? 🤨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How we "wrap" our web components into Angular is sorta special. We basically target the same selector tag as our web component, but then use Angular's content projection with
ng-content
to slot the existing web component inside the wrapping Angular component; since ng-content isn't an actual DOM node this results inion-modal
equalling our web component node.We then typically detach from all of Angular's change detection, because the updating of the web component should be driven through our implementation in Stencil and independent to anything in Angular. We don't want to cause an unwanted repaint of the web component. We only pass through inputs from our proxy to the web component node, so that the web component can decide when a re-render is necessary.
Overlays are a special condition (inline most specifically), because we are reliant on using
ng-template
since we are using template outlets (Angular feature). In Ivy, they fixedng-template
to have its own change detection reference to render changes inside just that template context. When we "detach" we are opting out of that behavior. With controller implementations the custom Angular component the implementer creates; would drive the change detection of the template (which wouldn't be detached by default).For all our other UI components, opting out makes sense. We aren't needing the benefits of functionality that Angular brings with what the web component needs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that's what those are for here! Thanks for the in-depth explanation 👍 If controller modals/popovers still work as expected (i.e. no extra repaints) then this sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to note this somewhere either in the Ionic wiki or in the comments in this code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a notion doc for now in our internal documents.