Repository navigation
Conversation
In the ionic theme with title-placement center, measure the slotted content with a ResizeObserver instead of a setTimeout with retries, so the slot sizes are set before the first paint and updated whenever the content changes size or a hidden toolbar becomes visible. Measure the slotted elements instead of the slot so the slots shrink when their content does, and call updateSlotClasses from the onSlotchange handlers.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summing the slotted elements' widths missed the space between them and did not match the slot when it shrinks, which changed the slot sizes (e.g. by 1px in WebKit). Measure the slot again, removing the size properties right before measuring so the slots can still shrink when their content does.
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me, solid fix! I left a test suggestion and a few comment nits.
Co-authored-by: Shane <shane.king@outsystems.com>
Co-authored-by: Shane <shane.king@outsystems.com>
Co-authored-by: Shane <shane.king@outsystems.com>
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! Just a couple of small readability nits on the latest changes, nothing that needs to hold this up.
| } | ||
|
|
||
| const elementsToObserve: Element[] = []; | ||
| const slots = ['start', 'end', 'primary', 'secondary']; |
There was a problem hiding this comment.
Nit: I think the new getSlottedElements() helper could absorb the other copies of this lookup. The slot list is now in three methods and the slot[name=...] query is repeated in a couple more, so if someone later edits one list and misses another, the observer ends up watching different elements than the ones being measured. A slotNames field next to slotClasses and a small getSlotElement(slotName) helper would cover all of them, and the toolbar tests still pass with that. Most of this predates your PR though, so no worries if you'd rather leave it!
| * again whenever that content changes size. | ||
| */ | ||
| private slotResizeObserver?: ResizeObserver; | ||
| private hasLoaded = false; |
There was a problem hiding this comment.
Nit: Could we call this didLoad? That's what ion-item, ion-menu and a few others use for the same guard, so it'd show up when someone searches for it. This is an extremely minor nit, just a basic consistency thing, so it's up to you
Issue number: N/A
What is the current behavior?
In the
ionictheme withtitle-placement="center", the toolbar centers the title by measuring thestart/end(andprimary/secondary) slots and writing the widest one to--start-end-size/--primary-secondary-size. Until then the slots have aflex-basisof0.The measurement runs in a
setTimeoutand retries every 100 ms, up to 5 times, while a slot measures0. This causes:onSlotchangehandlers returnthis.updateSlotClassesinstead of calling it, and buttons added to or removed from an existingion-buttonsdon't fireslotchangeat all.What is the new behavior?
ResizeObserverinstead of being measured in asetTimeout. Its callback runs after layout and before paint, so the slot sizes are set in the same frame the content is first laid out: the title is centered on the first painted frame. It also runs again whenever the slotted content changes size or a hidden toolbar becomes visible.onSlotchangehandlers now callupdateSlotClasses().center, and started again if the toolbar is added back.ResizeObservermeasure once.Does this introduce a breaking change?
Other information
Review focus
updateSlotWidths(): the first measurement comes from the observer's initial notification, which is delivered before the first paint.measureAndUpdateSlots()removes the size properties before measuring the slots. The image handling (--*-size: autowhile measuring) is unchanged.connectedCallback/disconnectedCallbackhandling of the observer.