Repository navigation
feat(label): convert to shadow DOM - #31657
brandyscarney wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The 3px comes from the markup change, not the shadow conversion. The h3 used to be the only child of ion-text, so iOS's h3:last-child { margin-bottom: 0 } applied. Shadow DOM can only style direct children, so it's now <h3><ion-text>…</ion-text></h3>. It's no longer a last child, so it keeps its 3px bottom margin. The scoped label renders this markup identically.
| <ion-text color="primary"> | ||
| <h3>H3 Title Text</h3> | ||
| </ion-text> | ||
| <h3><ion-text color="primary">H3 Title Text</ion-text></h3> |
There was a problem hiding this comment.
In shadow DOM, label's ::slotted() styles only reach its direct children, so a heading nested inside ion-text loses label's heading styles and gets the global typography instead. Moving ion-text inside the heading keeps the h3 a direct child, so it still gets label's styles. This is documented in BREAKING.md.
53e3491 to
9056bf4
Compare
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-stacked), | ||
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating), | ||
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-stacked), | ||
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-floating) { |
| :host(.label-floating) { | ||
| @include margin(null, null, 0, null); | ||
| @include transform(translate(0, 29px)); | ||
| @include transform-origin(start, top); |
There was a problem hiding this comment.
This was moved to label.scss.
| :host(.label-floating) { | ||
| @include margin(0, 0, 0, 0); | ||
| /* stylelint-disable property-disallowed-list */ | ||
| transform-origin: top left; |
There was a problem hiding this comment.
This was moved to label.scss.
ShaneK
left a comment
There was a problem hiding this comment.
Generally looks good, the conversion is really careful! The focused label color is the main one I'd like to see fixed, and I'm curious what you think about the CSS reset question. The rest are docs and nits.
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-stacked), | ||
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating), | ||
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-stacked), | ||
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-floating) { | ||
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating) { |
There was a problem hiding this comment.
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-stacked), | |
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating), | |
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-stacked), | |
| :host(.item-has-focus:not(.ion-color)) ::slotted(.label-floating) { | |
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating) { | |
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-stacked:not(.ion-color)), | |
| :host(.ion-focused:not(.ion-color)) ::slotted(.label-floating:not(.ion-color)) { |
A colored stacked or floating label turns primary when its item is focused in md now. For example, a color="danger" stacked label in an <ion-item button> or next to an ion-select goes to rgb(0, 84, 233) on keyboard focus, while on next it stays rgb(197, 0, 15). The removed label rule skipped colored labels, so this one needs the same :not(.ion-color). With the change it matches next again, and plain labels still get the focused color.
Could we add a test that checks a colored label keeps its color in a focused <ion-item button>? The "Do" example in the sass guidelines copies this rule too, so it should get the same change.
| ::slotted(ion-label.label-stacked), | ||
| ::slotted(ion-label.label-floating) { | ||
| @include margin(0, 0, 0, 0); | ||
| } |
There was a problem hiding this comment.
Floating labels don't float anymore when the item has item-has-value, which the Angular value accessor still adds, or when it's focused in md and ionic. On next the label scales up in both cases, and on this branch it stays in its resting position.
Should we add :host(.item-has-value) rules for ::slotted(ion-label.label-floating) to item's ios, md and ionic styles, plus :host(.ion-focused) for md and ionic? I checked that those match next. Or if we're dropping this along with the legacy sibling-label pattern, I think it should go in BREAKING.md.
| * The exclusion is wrapped in `:where()` to keep the specificity of a | ||
| * plain element selector so app styles can still override these rules. | ||
| */ | ||
| h1:where(:not(ion-label > h1)), |
There was a problem hiding this comment.
Have we thought about apps that use a CSS reset? A reset like Tailwind's preflight (font-size: inherit and margin: 0 on headings, inside @layer base) takes md label headings from 24/16/14px to 16px with no margins, and a Bootstrap-style reboot makes them 40/32/28px. Neither reset touched them on next, and apps can't add the :not(ion-label > h2) workaround to third-party CSS.
Document-level rules like ion-label.md > h1 would bring back the v8 behavior, but also the v8 limitation where ion-label h1 { } can't override them, which undoes the last bullet in BREAKING.md. I could go either way, which trade-off do you think we want?
|
|
||
| ### Parent Context | ||
|
|
||
| In scoped components, Stencil rewrites `:host-context()` into descendant selectors that work in every browser. In shadow components, the browser handles `:host-context()` itself, and only Chromium supports it. Styles that depend on an ancestor need a different approach after converting. |
There was a problem hiding this comment.
The "IN SHADOW" examples just above this still use :host-context(ion-toolbar.ion-color), which this section says only works in Chromium. Could we update them to match segment? It sets 'in-toolbar-color': hostContext('ion-toolbar[color]', this.el) and targets :host(.in-toolbar-color:not(.ion-color)), like in the segment iOS styles.
| :host(.in-item) { | ||
| ``` | ||
|
|
||
| Combine a tag name with a class only when the class describes the ancestor's state, such as `hostContext('ion-item.ion-color', this.el)`. |
There was a problem hiding this comment.
I think this example is the case the next paragraph warns about. Since ion-color can change after render, setting the item's color once the label has rendered leaves in-item-color unset, which is also how it behaves on next. Maybe add a short note that it won't update?
| font-size: dynamic-font(22px); | ||
| font-weight: normal; | ||
|
|
||
| line-height: 1.2; |
There was a problem hiding this comment.
Nit: a lot of this is identical between the iOS and md stylesheets, like the h3-h6 font rules, the h1/h2 weight and line height, and the paragraph overflow and in-item-color rules. I think those could move to label.scss, and I checked it doesn't change how anything renders. A short comment on line-height: 1.2 would help too, since it replaces the global heading line height that no longer reaches label headings. Up to you!
| @@ -1,3 +1,5 @@ | |||
| // TODO(FW-6850): Remove when label is migrated to Modular Ionic. | |||
| @use "../../foundations/ionic.vars" as ionic; | |||
There was a problem hiding this comment.
Nit: other components read the ionic tokens through @use "../../themes/ionic/ionic.globals.scss" as globals;, like the solid input styles. Could we use that here and switch the two map-get calls below to globals.? The compiled rules come out the same.
| @@ -111,10 +111,10 @@ export class Label implements ComponentInterface { | |||
| <Host | |||
| class={createColorClasses(this.color, { | |||
There was a problem hiding this comment.
Nit: since this render was touched, could we destructure at the top per FW-7759? Something like const { color, el, noAnimate, position } = this;, then use those here instead of this.color, this.el and this.noAnimate.
Issue number: internal
What is the current behavior?
ion-labelusesscopedencapsulation.What is the new behavior?
ion-labelto Shadow DOM.::slotted(), so heading and paragraph styles now apply toh1-h6andpelements that are direct children ofion-label.typography.cssandtypography.ionic.cssnow skip headings that are direct children ofion-label, using selectors likeh1:where(:not(ion-label > h1))for each heading level, so the label's heading styles apply to them. The:where()keeps the original specificity.::slotted(ion-label)styles take precedence over label's own styles in shadow DOM.:host-context, which only Chromium supports in shadow DOM.transform-originmixin instead of a render-timelabel-rtlclass.:host-contextrules for item classes that are no longer set (item-input,item-textarea,item-has-focus,item-has-value,item-has-placeholder), along with the focused and invalid color rules for stacked and floating labels.isRTL()checked when needed, instead of render-time classes.Does this introduce a breaking change?
BREAKING CHANGE:
ion-labelhas been converted to use Shadow DOM.h1-h6andpelements that are direct children ofion-label. Nested headings, such as<ion-label><ion-text><h3>, no longer get label styles and get the global typography instead. Moveion-textinside the heading or paragraph, or setcoloronion-label.h2 { font-size: 20px; }now take precedence over label's heading and paragraph styles. Scope them withh2:not(ion-label > h2)to keep the label styles.ion-labeldirectly, such as a class that setscolor, now take precedence over the default label styles.Other information
:not()and:where()selectors in the global typography need Firefox 84+ and 78+, and docs(breaking): add minimum browser versions for v10 #31605 raises the documented minimum to Firefox 89.