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!
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
fe6013d to
73cecc7
Compare
# Conflicts: # core/api.txt # core/src/components/title/title.scss
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good, a few requests but nothing major!
| /** | ||
| * The large title grows from the edge its text is aligned to, so the | ||
| * text's start doesn't shift. Centered text grows from the center. | ||
| * | ||
| * @internal | ||
| * @param ionTitleEl - The title whose alignment and direction set the origin. | ||
| * @returns The horizontal `transform-origin` keyword: `left`, `right` or `center`. | ||
| */ | ||
| const getTitleOrigin = (ionTitleEl: HTMLIonTitleElement): string => { | ||
| const { textAlign, direction } = getComputedStyle(ionTitleEl); | ||
| const rtl = direction === 'rtl'; | ||
|
|
||
| if (textAlign === 'center' || textAlign === '-webkit-center') { | ||
| return 'center'; | ||
| } | ||
|
|
||
| if (textAlign === 'left' || textAlign === 'right') { | ||
| return textAlign; | ||
| } | ||
|
|
||
| if (textAlign === 'end') { | ||
| return rtl ? 'left' : 'right'; | ||
| } | ||
|
|
||
| return rtl ? 'right' : 'left'; | ||
| }; |
There was a problem hiding this comment.
Nothing tests getTitleOrigin automatically. Could you add a spec covering start, end, left, right and center in LTR and RTL? Also, since it’s cached at index creation, setting size="large" later writes undefined center. Would working it out lazily in scaleLargeTitles avoid that?
# Conflicts: # core/src/components/title/title.interface.ts
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me, great work! Just a few comments, mostly around test coverage, so feel free to take or leave them.
| * How far the drawn text sits from where the shrunk box starts: 0 for a | ||
| * start-aligned title, more for a centered one. The copies start there. | ||
| */ | ||
| const startShift = Math.max(0, rtl ? box.right - text.right : text.left - box.left); |
There was a problem hiding this comment.
| const startShift = Math.max(0, rtl ? box.right - text.right : text.left - box.left); | |
| const startShift = text.width === 0 ? 0 : Math.max(0, rtl ? box.right - text.right : text.left - box.left); |
When the large title is empty, the range rect is all zeros, so in RTL this comes out as box.right and the back button copy starts on the wrong side of the screen. With "Back" as the back text, its text starts around x = 1 in ios and -32 in ionic instead of near 314. Falling back to 0 puts it back on the right, and the transition tests still pass.
| await expect(page.locator('page-inbox ion-title[size="large"]')).toHaveClass(/hydrated/); | ||
| }; | ||
|
|
||
| test(`should start the large title copy on the title text in the ${theme} theme`, async ({ page }) => { |
There was a problem hiding this comment.
Both position tests stop on the start keyframe, so removing largeTitleStartShift from both END_TRANSLATE_X branches and from CONTAINER_START_TRANSLATE_X still passes everything here. I tried an exact end check, but it's 7-11px off in md and ionic even on this branch, which makes sense since the copies only roughly overlap and the forward copy has faded out by then. Do you think a check with a loose tolerance is worth adding?
|
|
||
| [ | ||
| { label: 'default', color: undefined, hue: undefined }, | ||
| { label: 'bold primary', color: 'primary', hue: 'bold' }, |
There was a problem hiding this comment.
I don't think this test checks the font copy at all, since removing only font-family still passes everything. Removing color too still passes bold primary in every theme, because the copy already gets color and falls back to the bold hue. Could a case where the toolbar uses a different font than body cover it?
| scaleLargeTitles(headerIndex.toolbars, 1.1); | ||
|
|
||
| const innerTitle = title.shadowRoot!.querySelector<HTMLElement>('.toolbar-title')!; | ||
| expect(innerTitle.style.transformOrigin).not.toContain('undefined'); |
There was a problem hiding this comment.
| expect(innerTitle.style.transformOrigin).not.toContain('undefined'); | |
| expect(innerTitle.style.transformOrigin).toBe('left center'); |
This passes on an empty string too, so it wouldn't catch scaleLargeTitles skipping the write. Mock-doc has no textAlign, so it's always left center here, and this version fails both with the write removed and with the old undefined center caching.
| 'padding-right': paddingRight, | ||
| 'padding-bottom': paddingBottom, | ||
| 'padding-left': paddingLeft, | ||
| 'pointer-events': 'none', |
There was a problem hiding this comment.
| 'pointer-events': 'none', |
I don't think this one does anything. The host already has pointer-events: none from the stylesheet and .toolbar-title sets it back to auto, so the copy's text takes hits either way. If the copy should be inert like the back button copy, setting it on the copy's .toolbar-title next to the width: auto write does that.
| * so a centered title never shows off-center. Runs again on slotchange | ||
| * to watch content added later. | ||
| */ | ||
| private observeSlotContent() { |
There was a problem hiding this comment.
Since getTitlePlacement() returns center for ios too, every ios theme toolbar gets this observer, but nothing there reads the classes or --start-end-size. The slots are display: contents in ios, so they always measure 0 and the variable never gets set. Is that fine to leave until FW-6864, or should it be limited to ionic for now?
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.