fix(transition): ios transition works on devices without IO support #21339
Conversation
There was a problem hiding this comment.
Thanks for the PR! I am not sure this is the best approach to fixing this bug. If IntersectionObserver is not supported, the collapsable header should not even be activated. In that case, a check here should not even be necessary.
Maybe we can have some logic here that checks for API support? https://github.com/ionic-team/ionic/blob/master/core/src/components/header/header.tsx#L145
Without intersection observer support, the collapsable header is pretty much useless so I am not sure it's even worth it to have the class applied.
|
@liamdebeasi Hm I'm not sure we're 100% talking about the same thing here. I agree it's useless to setup the collapse animation without IO support. The root problem is that the animation logic is completly separated from the component (in the iOS transition), and it is accessing the cloned title and back button elements for example: These are only created if the collapsible header is setup - but as it is not setup anymore (this was fixed by you in #21222) this leads to a TypeError. At first I wanted to skip creating the large title animation at all, but the return values are used further down the pipe to determine if we're animating forward or backward: So yeah, I it's not the prettiest fix, but it does the job. |
|
Oh hmm... Maybe it should still create the cloned title regardless of IO support: https://github.com/ionic-team/ionic/blob/master/core/src/components/header/header.tsx#L129-L132. The cloned elements are really more for the large title transition than it is for the collapsable header. If we did that, then the check in the iOS transition file should not be necessary. |
|
@liamdebeasi Yeah sure, that should work as well, I'll move the cloning parts before the early exit and revert the check in the transition tomorrow as soon as I'm back at my dev machine! Or feel free to close my PR if you want to do it yourself :) |
|
I'll stick with this PR -- will take another look when the changes are made, but I think we should be good to merge after. |
|
Great! Thanks for the feedback so far, I‘ll let you know when I‘m done! |
|
@liamdebeasi This is ready for review again :) |
|
Merged. Thank you! |
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 following error is thrown on every page transition if using collapsible headers on a device without IntersectionObserver support:
TypeError: null is not an object (evaluating 'clonedTitleEl.innerText = largeTitleEl.innerText')Issue Number: fixes #21221
What is the new behavior?
The part of the the page transition where the cloned element would be needed is skipped in case the IO API is not available.
Does this introduce a breaking change?