feat(header, footer, tab-bar): add scrollEffect prop#31263
feat(header, footer, tab-bar): add scrollEffect prop#31263OS-susmitabhowmik wants to merge 60 commits into
Conversation
…rrect theme class usage
…orce footer priority
…llEffect in all modes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
package: core
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Hey, great work on this! I highlighted a few issues I found, some of them aren't as important as others, but still
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Thanks for grinding through all of this, most of the last round holds up and I verified it. Found a few more edges though, including one where the main header disappears with no large title (reproduced it inline). Nothing structural, mostly tightening up the cases we keep uncovering. Let me know if I'm off base on any of these!
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Hey, I think we're finally getting close to getting this one wrapped up! Just a few more issues I noticed, but we're so close!
| scrollEl.addEventListener('scroll', handleScroll, { passive: true }); | ||
| scrollEl.addEventListener('wheel', handleWheel as EventListener, { passive: true }); | ||
|
|
||
| contentEl.classList.add(contentPartnerClass); |
There was a problem hiding this comment.
Heads up, I think the consolidation reopened a concurrency hole here. In the old per-component code all the shared-content setup (the partner class, the cssVar, the ResizeObserver) lived behind the setupHidePromise === promise guard, so a superseded setup was inert and never touched ion-content. Now the controller runs all of it unconditionally before it resolves, and the caller tears a loser down with destroy(), which removes contentPartnerClass and the cssVar off the shared content (lines 214-215).
Since getScrollElement awaits componentOnReady then the content's own async getter, two overlapping setups on the same content can resolve out of order, so the loser's destroy() can land after the winner's setup and strip the class + var the winner still needs. Per content.scss that kills the gap compensation, so you get a blank gap that toggles on scroll. Tab-bar is the easiest to hit since checkScrollEffect has no activeEffect guard and always destroys + rebuilds; header/footer reopened it by moving activeEffect = effect to after the await.
I think the loser should only tear down what it uniquely owns (its listeners/observer), or we hold the shared-content mutations until the caller confirms the setup is current like the old guarded version did. Restoring activeEffect before the await would also reclose the header/footer window. Let me know if I'm misreading the ordering here!
| const pageEl = this.el.closest(`${appRootSelector},ion-page,.ion-page,page-inner`); | ||
| const contentEl = pageEl ? findIonContent(pageEl) : null; | ||
|
|
||
| this.activeEffect = effect; |
There was a problem hiding this comment.
This is the one path where activeEffect still gets stamped after a setup that can bail. setupCondenseHeader has a few early returns (no content/page, no IntersectionObserver, no large title, no main header), but this.activeEffect = effect runs regardless, and then the effect === this.activeEffect guard up top blocks the retry on a later re-render. Same thing we fixed on the hide and fade paths. Could setupCondenseHeader report whether it actually set up, so a deferred large-title or content case still retries?
|
|
||
| export type ScrollHideController = { | ||
| /** Whether the component is currently hidden by the scroll effect. */ | ||
| readonly isHidden: boolean; |
There was a problem hiding this comment.
Could this expose isHidden as an accessor like createKeyboardController does with isKeyboardVisible? Right now the state lives in both the controllerIsHidden closure and controller.isHidden, gets written in two places, and needs the as ScrollHideController cast to satisfy the readonly on a value it mutates. A () => controllerIsHidden getter collapses it to one source of truth and drops both the cast and the forward reference to controller from inside setHidden. The only caller is tab-bar.tsx.
| const effect = scrollEffect ?? collapse; | ||
| // condense/fade via the deprecated `collapse` prop are iOS-only. | ||
| const isModeRestricted = scrollEffect === undefined && theme !== 'ios'; | ||
| const hasLargeTitle = this.el.querySelector('ion-title[size="large"]') !== null; |
There was a problem hiding this comment.
Nit: this runs a DOM querySelector on every render. Headers don't re-render often so it's minor, but could it be cached or driven off a slot change instead?
3 participants
Issue number: resolves internal
What is the current behavior?
ion-headerandion-footerhave acollapseprop for scroll-driven effects (condense,fade), but it only works on the iOS theme.ion-tab-barhas an unreleased hideOnScroll boolean prop that only works on the Ionic theme withexpand="compact".What is the new behavior?
scrollEffectstring enum prop toion-header,ion-footer, andion-tab-bar.scrollEffect="hide"slides the bar out of view when scrolling down and back in when scrolling up.scrollEffect="condense"andscrollEffect="fade"onion-header/ion-footerreplace the existingcollapsevalues and work across all themes.collapseprop onion-headerandion-footerwith a console warning directing developers to usescrollEffect.hideOnScrollprop fromion-tab-bar.ion-tab-baris nested insideion-footer, the footer owns the hide animation to prevent double-animationDoes this introduce a breaking change?
Other information
Relevant Test Pages:
Header
Deprecated
collapsepropscrollEffectpropFooter
Deprecated
collapsepropscrollEffectpropTab Bar