Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The title in this snapshot now follows the page's md theme instead of the header's mode="ios", since migrated components take their look from the theme and mode only affects behavior; the header itself is still visible, which is what the test checks.
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good! I requested some changes. 🙂
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good just one optional change in the interface!
# Conflicts: # core/src/components/title/test/color/title.e2e.ts
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
# Conflicts: # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ionic-md-ltr-light-Mobile-Chrome-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ionic-md-ltr-light-Mobile-Firefox-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ionic-md-ltr-light-Mobile-Safari-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ios-ltr-Mobile-Chrome-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ios-ltr-Mobile-Firefox-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-ios-ltr-Mobile-Safari-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-md-ltr-Mobile-Chrome-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-md-ltr-Mobile-Firefox-linux.png # core/src/components/title/test/color/title.e2e.ts-snapshots/title-color-md-ltr-Mobile-Safari-linux.png
ShaneK
left a comment
There was a problem hiding this comment.
Looks really good overall! Just a few things, mainly the transition copy losing hue and the two visual changes missing from the breaking changes doc. The rest are smaller.
| @@ -17,7 +17,7 @@ export const shadow = <T extends Element>(el: T): ShadowRoot | T => { | |||
|
|
|||
| const getLargeTitle = (refEl: any) => { | |||
| const tabs = refEl.tagName === 'ION-TABS' ? refEl : refEl.querySelector('ion-tabs'); | |||
| const query = 'ion-content ion-header:not(.header-collapse-condense-inactive) ion-title.title-large'; | |||
| const query = 'ion-content ion-header:not(.header-collapse-condense-inactive) ion-title.title-size-large'; | |||
There was a problem hiding this comment.
The copy of the large title used in this transition gets size and color from the real title in animateLargeTitle, but not the new hue. So a large title with color="primary" hue="subtle" renders at rgb(0, 74, 205), but its copy animates in the bold color, rgb(0, 84, 233), on both push and pop and then switches back when the real title shows again. Copying hue across keeps it subtle the whole way through:
clonedTitleEl.color = largeTitleEl.color;
clonedTitleEl.hue = largeTitleEl.hue;| bold: { | ||
| semantic: { | ||
| default: { | ||
| color: currentColor('base'), |
There was a problem hiding this comment.
Is base intentional here? The subtle hue reads foreground, and ion-text uses foreground for both hues since it moved off base in #31170. With primary on ios, a bold title comes out rgb(0, 84, 233) and a subtle one rgb(0, 74, 205), so subtle is actually darker than bold, and a primary title doesn't match a primary ion-text either. If it's meant to keep the current look that's fine, otherwise md and ionic have the same line.
| 3. The size classes now include the property name, and a title without `size` is now `medium`. <sup>[3](#version-10x-title-size-classes)</sup> | ||
| 4. The `title-rtl` class has been removed. <sup>[4](#version-10x-title-rtl-class)</sup> | ||
| 5. Theme classes (`ion-title.md`, `ion-title.ios`) are no longer supported. <sup>[5](#version-10x-title-theme-classes)</sup> | ||
| 6. In the `md` theme, a tap on the title's padding no longer counts as a tap on the title. <sup>[6](#version-10x-title-padding-taps)</sup> |
There was a problem hiding this comment.
I think the iOS small title padding (9px to 16px) and the md letter spacing (0.0125em to 0.0075em) should go in here too, since Button, Card and Chip all list their default visual changes. Setting --ion-title-size-small-padding-start and --ion-title-size-small-padding-end to 9px, or --ion-title-letter-spacing to 0.0125em, brings the old look back, so those could be the revert path.
| // TODO(FW-6864): remove for small and medium when ion-toolbar titlePlacement works in every theme (ios center, md start) | ||
| position?: string; |
There was a problem hiding this comment.
Could we drop position for large too? Only the medium title is absolutely positioned now, so the static iOS sets for large is just the default, and unsetting it doesn't move the title at all. The TODO only removes it for small and medium, so large would keep position as a public token after FW-6864, which I think is what Brandy wanted to avoid.
| :host(.toolbar-title-size-large) .toolbar-container { | ||
| flex-wrap: wrap; | ||
| align-items: flex-start; | ||
| } | ||
|
|
||
| :host(.toolbar-title-large) .toolbar-content ion-title { | ||
| flex: 1; | ||
| order: map-get($toolbar-order-ios, title-large); | ||
|
|
||
| // The large title fills a row of its own, so the wrap moves it below the buttons | ||
| :host(.toolbar-title-size-large) ::slotted(ion-title) { | ||
| min-width: 100%; | ||
| } |
There was a problem hiding this comment.
| :host(.toolbar-title-size-large) .toolbar-container { | |
| flex-wrap: wrap; | |
| align-items: flex-start; | |
| } | |
| :host(.toolbar-title-large) .toolbar-content ion-title { | |
| flex: 1; | |
| order: map-get($toolbar-order-ios, title-large); | |
| // The large title fills a row of its own, so the wrap moves it below the buttons | |
| :host(.toolbar-title-size-large) ::slotted(ion-title) { | |
| min-width: 100%; | |
| } | |
| :host(.toolbar-title-size-large) .toolbar-container { | |
| flex-wrap: wrap; | |
| align-items: flex-start; | |
| } |
I know this came out of an earlier suggestion, but I don't think it does anything. The title is slotted into .toolbar-content, so it isn't a flex item of the wrapping .toolbar-container and can't be pushed onto its own row. With start and end buttons, a long title or the condense header, the large title stays on the same row either way, and removing the rule doesn't change any title, header or toolbar screenshot. The old rule in title.ios.scss didn't do anything either, so this isn't a regression.
| @@ -33,7 +33,7 @@ | |||
| </ion-header> | |||
|
|
|||
| <ion-header> | |||
| <ion-toolbar class="toolbar-title-default"> | |||
| <ion-toolbar class="toolbar-title-size-medium"> | |||
There was a problem hiding this comment.
The setContent copy of this markup in title.e2e.ts still has the old toolbar-title-default class:
<ion-toolbar class="toolbar-title-size-medium">Renaming it there doesn't change any of the basic screenshots. This is just a nit though, up to you!
Issue number: resolves internal
What is the current behavior?
ion-titledoes not fragment styles based on themes.What is the new behavior?
ion-titleto Modular Ionic: one token-driven stylesheet,IonTitleRecipeandIonTitleConfig, and per-theme token defaults for ios, md and ionic.hueprop (boldby default, orsubtle). The ionic theme now appliescolor, matching ios and md. The hue test covers every color in ios, md and ionic, so the separate color test is removed.sizeacceptsmedium, which is the default from the newIonTitle.sizeconfig. The header's condense effect also reads that config, so a title made large through it condenses. Size classes are nowtitle-size-small,title-size-mediumandtitle-size-large, and toolbar's matching classes aretoolbar-title-size-*.--colorwith--ion-title-color, which also reaches the copy of a large title shown during the iOS page transition.getIonThemeand thethemevirtual prop) and thetitle-rtlclass.box-sizing: border-boxandpointer-events: noneare set in the stylesheet for every theme instead of through tokens. In md, a tap on the title's padding now reaches the toolbar, as it already did in ios and ionic.min-width: 100%moves totoolbar.ios.scss, next to theflex-wraprule it works with.--ion-spacing-lg(16px) instead of9px, matching the navigation bar prompt in Apple's iOS 17 and 18 design kits. Long small titles wrap a little sooner.0.0075em(0.15px at 20px) instead of0.0125em, matching Headline 6 in Google's Material 2 design kit. The old value came from a 2019 toolbar update (fix(toolbar): update md toolbar button spacing and segment padding #17537).title-largeorder value. The rule's selector can't reach the slotted title, so it never applied, and the title already computesorder: 0. Rendering doesn't change.dynamicFontMaxtoken helper, and rounds scaled sizes indynamicFontMinanddynamicFontMax.title-basicscreenshots change where the long small title wraps. The hue screenshots now include ios. In header's "root iOS header in an MD app" screenshot, the title follows the page's theme: an ancestormodeattribute now only affects behavior, as it already does for every migrated component.Does this introduce a breaking change?
ion-titleCSS variables and classes are replaced by token-driven custom properties and prop-named classes. Use the global token for theme-wide styling, or the CSS variable for component-specific overrides.--colorIonTitle.color--ion-title-colortitle-defaulttitle-size-mediumtitle-smalltitle-size-smalltitle-largetitle-size-largetoolbar-title-defaulttoolbar-title-size-mediumtoolbar-title-smalltoolbar-title-size-smalltoolbar-title-largetoolbar-title-size-largeRTL class:
title-rtlis removed. Useion-title:dir(rtl)instead.Theme classes: remove any instances that target
ion-title.mdorion-title.ios.Taps on the title's padding: in md, a tap on the padding at the start or end of
ion-titlenow reaches the element behind it, usuallyion-toolbar. Listen onion-toolbarif those taps need to count.Other information
Left for the parents
Some
ion-titlestyles still live in, or depend on, its parents. They move with that parent's work, so they aren't part of this PR.text-alignrules intoolbar.ionic.scss, the iOS title's own centering and its 90px side padding intitle.scss(markedTODO(FW-6864)), and the iOS large title's full-width row intoolbar.ios.scss. Once the toolbar places the title in every theme, the title's centering and side padding can go.header.common.scssthat hide and fade the title as the large title collapses. They depend on the header's scroll state and include its condense motion, so they move in the header migration (FW-6843).How to test
Open each before and after link side by side.