fix(datetime): timepicker popover will position relative to click target #24616
Conversation
| this.isTimePopoverOpen = true; | ||
| popoverRef.present(ev); | ||
|
|
||
| popoverRef.present(new CustomEvent('ionShadowTarget', { |
There was a problem hiding this comment.
Should we extend ev instead of overwriting it completely? Similar to what ion-select does here: https://github.com/ionic-team/ionic-framework/blob/main/core/src/components/select/select.tsx#L336
There was a problem hiding this comment.
After navigating what was used of the event object in the present function, it appeared to only use the event target or the shadow target (if set).
Was mainly following the feedback here: #24535 (comment)
I could see merging the objects on the chance that we need additional properties in the future 🤔
| const cols = (ev.target! as HTMLElement).querySelectorAll('ion-picker-column-internal'); | ||
| cols.forEach(col => col.scrollActiveItemIntoView()); |
There was a problem hiding this comment.
Assuming this can be removed if we figure out a way to remove the IOs completely, it might be worth adding a note about that somewhere. (Tech debt comment?)
There was a problem hiding this comment.
I believe this should replace the IO behavior, but believe further discovery is needed to fully vet that out (created a spike for that, referencing this PR).
The picker column internal component is only used in context of the datetime and only shown within a popover, so the popover should be able to dictate all visibility-related functions with lifecycle events, instead of relying on an inner IO. I'm just unsure if picker-column-internal is intended to move to the public API for available components.
There was a problem hiding this comment.
Ultimately I'd like for picker-column-internal to replace the public picker-column since there are many issues with the current implementation. However, I think that is a ways off, so it should be fine to roll with this approach for now.
The only thing I'd ask is we add a TODO and make tech debt to investigate a way of having this be handled automatically. If we make this component public, I imagine users will run into this same issue.
| const cols = (ev.target! as HTMLElement).querySelectorAll('ion-picker-column-internal'); | ||
| cols.forEach(col => col.scrollActiveItemIntoView()); |
There was a problem hiding this comment.
Ultimately I'd like for picker-column-internal to replace the public picker-column since there are many issues with the current implementation. However, I think that is a ways off, so it should be fine to roll with this approach for now.
The only thing I'd ask is we add a TODO and make tech debt to investigate a way of having this be handled automatically. If we make this component public, I imagine users will run into this same issue.
Resolves #24531, #24415 Co-authored-by: mixalbl4 <[email protected]>
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
The popover positioning within datetime can result in the popover element being placed outside of the trigger/click target. Additionally, rapidly opening the timepicker within the datetime, can result in the timepicker not scrolling to the correct time.
Issue Number: #24531, #24415
What is the new behavior?
ionShadowTargetto consistently position the popover when the click target/trigger is within a shadow DOM.Does this introduce a breaking change?
Other information
Extension of #24535 (will close in favor of this, if accepted).
Popover positioning
Kapture.2022-01-20.at.15.51.48.mp4
Kapture.2022-01-20.at.15.52.58.mp4
Timepicker columns (combined with above fix)
Kapture.2022-01-20.at.15.48.47.mp4
Kapture.2022-01-20.at.15.46.17.mp4