Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
5974626 to
04dc471
Compare
04dc471 to
fe6013d
Compare
brandyscarney
left a comment
There was a problem hiding this comment.
Added a few minor requests but this is looking good!
There was a problem hiding this comment.
Why did the tests lose padding on this branch? It doesn't look like the test itself changed, are we not respecting the ion-padding class?
| } | ||
|
|
||
| // The slotted content, not the slots, so setting the slot widths doesn't re-trigger it | ||
| this.el.querySelectorAll(':scope > [slot]').forEach((el) => this.slotContentObserver!.observe(el)); |
There was a problem hiding this comment.
This only observes slot content that exists when it runs (at load, or when titlePlacement changes), so buttons added later with *ngIf or conditional rendering aren't watched. The has-*-content classes already had this limitation, so it isn't a regression. But the comment above this function says it handles "a back button that renders after the toolbar", which is only true if the element is already in the DOM. Could you either reword the comment or watch for slot changes too?
|
|
||
| disconnectedCallback() { | ||
| this.slotContentObserver?.disconnect(); | ||
| this.slotContentObserver = undefined; |
There was a problem hiding this comment.
This sets the observer to undefined on disconnect, but it's only created again in updateSlotClasses(), which runs from componentWillLoad and componentDidLoad. Stencil runs those once, so if the toolbar is removed and re-inserted (framework portals, Vue , a cached page moved in the DOM), the centering fix stops working. Could you recreate it in connectedCallback, for example by calling this.observeSlotContent() there when getTitlePlacement() === 'center'?
| background: toolbar.shadowRoot!.querySelector('.toolbar-background'), | ||
| ionTitleEl, | ||
| innerTitleEl: ionTitleEl ? ionTitleEl.shadowRoot!.querySelector('.toolbar-title') : null, | ||
| innerTitleOrigin: ionTitleEl ? getTitleOrigin(ionTitleEl) : undefined, |
There was a problem hiding this comment.
Nit: this runs getComputedStyle on every toolbar title in the header, but only large titles use the origin (scaleLargeTitles returns early otherwise). Could it be:
| innerTitleOrigin: ionTitleEl ? getTitleOrigin(ionTitleEl) : undefined, | |
| innerTitleOrigin: ionTitleEl && isLargeTitle(ionTitleEl) ? getTitleOrigin(ionTitleEl) : undefined, |
There was a problem hiding this comment.
Could you add two cases here? Both cover behavior this PR moved out of CSS and into JavaScript, and nothing else tests them:
- Backward transition: going back to the large-title page, which is the most common case.
largeTitleStartShiftchanged the start and end math in both directions, but only forward is tested, and backward uses a different measurement (the entering title instead of the leaving one). - Color and font on the copy: the removed
:host(.title-size-large.ion-cloned-element)rule is now replaced by copyinggetComputedStylevalues. Could you check that the copy's computedcolorandfont-familymatch the title's, withcolor="primary"and both hues ("Cycle title color" does this), including the ionic theme since it sets its own font? Screenshots won't catch this because they never capture a transition in progress.


Issue number: resolves internal
What is the current behavior?
The iOS large title animations rely on the iOS theme's title stylesheet, so they only look right in the iOS theme. With iOS mode and the
mdorionictheme, the page transition squashes the title (width scale 0.20 instead of about 0.5), a centered title jumps to the screen edge, and the overscroll stretch slides the title 17px past its padding.What is the new behavior?
visibilityso a title placed next to it doesn't shift.This is the first proof of concept for FW-7295: animations set up what they need instead of relying on a theme's stylesheet.
Does this introduce a breaking change?
Other information
Open each before and after link side by side. For the pull down rows, use Safari on a Mac with a trackpad in Responsive Design Mode for a phone-sized view, or the iOS Simulator. With the page at the top, do a two-finger swipe down over the page content so it bounces past the top. Chrome and Firefox, including their device modes, can't scroll past the top, so the title won't grow there.