fix(popover): dynamic width popover is positioned correctly #28072
Conversation
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
|
|
liamdebeasi
commented
Aug 29, 2023
|
|
||
| this.ionMount.emit(); | ||
|
|
||
| this.usersElement = await attachComponent(delegate, el, this.component, ['ion-page'], this.componentProps, inline); |
There was a problem hiding this comment.
Moving ionMount above attachComponent isn't strictly necessary for modal, but I decided to make the change so we are at least consistent with ion-popover.
liamdebeasi
changed the title
liamdebeasi
requested review from
a team and
thetaPC
and removed request for
a team
averyjohnston
approved these changes
Aug 30, 2023
thetaPC
approved these changes
Aug 30, 2023
| @@ -449,6 +449,9 @@ export class Popover implements ComponentInterface, PopoverInterface { | |||
| const { el } = this; | |||
|
|
|||
| const { inline, delegate } = this.getDelegate(true); | |||
|
|
|||
| this.ionMount.emit(); | |||
There was a problem hiding this comment.
It might be beneficial to add a comment to why it's important to have ionMount above attachComponent to prevent it being moved to a lower position.
There was a problem hiding this comment.
Good call! Added in c4beade.
This was referenced Aug 31, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue number: resolves #27190, resolves #24780
What is the current behavior?
Popovers with dynamic widths were not being positioned correctly relative to the trigger element. This was happening because the child component always had dimensions of 0 x 0. Ionic has logic built-in to wait for the child components to be rendered, but this was not working as intended for two reasons:
this.usersElementwas referencing the popover element itself not the user’s component. When callingdeepReadyonionic-framework/core/src/components/popover/popover.tsx
Line 477 in 01fc9b4
This is happening because we return
BaseComponentfromattachComponentwhich is a reference to the overlay:ionic-framework/core/src/utils/framework-delegate.ts
Line 133 in 01fc9b4
Other framework delegates return the actual child content:
ionic-framework/core/src/utils/framework-delegate.ts
Line 35 in 01fc9b4
ionic-framework/packages/react/src/framework-delegate.tsx
Line 31 in 01fc9b4
ionic-framework/packages/vue/src/framework-delegate.ts
Line 45 in 01fc9b4
attachComponentis unable to return the correct element currently because the child content has not been mounted yet in this scenario.ionMountis emitted afterattachComponentresolves:ionic-framework/core/src/components/popover/popover.tsx
Line 466 in 01fc9b4
What is the new behavior?
ionMountis emitted beforeattachComponentrunsattachComponentnow consistently returns the child view if present in the DOMDoes this introduce a breaking change?
Other information
Dev build:
7.3.2-dev.11693321763.15a54694