Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
# Conflicts: # core/src/components/button/button.scss
ShaneK
left a comment
There was a problem hiding this comment.
Looks really good, the subtle tokens line up with bold in every theme! Just a couple of things, mainly the datetime and password toggle buttons picking up a subtle hue from the config.
| */ | ||
| get hueValue(): Hue { | ||
| return config.getObjectValue('IonButton.hue', 'bold') as Hue; | ||
| const hueConfig = config.getObjectValue('IonButton.hue', 'bold') as Hue; |
There was a problem hiding this comment.
Setting IonButton.hue to subtle in the theme config also reaches the buttons inside ion-datetime and ion-input-password-toggle, since they don't set hue, and their overrides only cover bold. With that config the md datetime month arrows go from gray rgb(89, 89, 89) to rgb(0, 74, 205), and the ionic password toggle goes from rgb(108, 114, 122) to the same blue. Could those internal buttons pass hue="bold", or would you rather add subtle overrides like the toolbar follow-up?
There was a problem hiding this comment.
Went with hue="bold" in ccbad11. Datetime's five buttons and the password toggle only get styled through the bold tokens, so a configured hue shouldn't reach them. With IonButton.hue set to subtle, the md month arrows stay rgb(89, 89, 89) and the ionic password toggle stays rgb(108, 114, 122), while regular buttons still follow the config.
There was a problem hiding this comment.
Thanks, that fixes it! Could we add tests for this though? Nothing covers the hue="bold" on those six buttons or the this.hue ?? hueConfig order, so dropping one of the attributes or flipping the ?? would bring the subtle month arrows and toggle back with every test still passing. A button.spec.ts case with IonButton.hue: 'subtle' in the config and hue="bold" on the button would cover the precedence, and datetime and password toggle specs with the same config that check their buttons keep button-hue-bold would cover the attributes.
# Conflicts: # BREAKING.md # core/src/components/button/button.scss
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the fixes, just a couple of small follow-ups on the tests!
| */ | ||
| get hueValue(): Hue { | ||
| return config.getObjectValue('IonButton.hue', 'bold') as Hue; | ||
| const hueConfig = config.getObjectValue('IonButton.hue', 'bold') as Hue; |
There was a problem hiding this comment.
Thanks, that fixes it! Could we add tests for this though? Nothing covers the hue="bold" on those six buttons or the this.hue ?? hueConfig order, so dropping one of the attributes or flipping the ?? would bring the subtle month arrows and toggle back with every test still passing. A button.spec.ts case with IonButton.hue: 'subtle' in the config and hue="bold" on the button would cover the precedence, and datetime and password toggle specs with the same config that check their buttons keep button-hue-bold would cover the attributes.
| .row { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| align-items: center; | ||
| gap: 4px; | ||
| } |
There was a problem hiding this comment.
| .row { | |
| display: flex; | |
| flex-wrap: wrap; | |
| align-items: center; | |
| gap: 4px; | |
| } | |
| .row { | |
| display: flex; | |
| flex-wrap: wrap; | |
| align-items: center; | |
| gap: 10px; | |
| margin-block-end: 10px; | |
| } |
With a 4px gap the ionic focus rings overlap the neighboring buttons, both side to side and between wrapped rows, since the ring reaches 4px outside the button. That makes the ionic-md hue screenshots hard to read when someone changes the focus ring later. A 10px gap matches the states test and keeps every ring at least 2px clear. The hue snapshots would need regenerating with this.
# Conflicts: # BREAKING.md # core/src/components/button/button.scss # core/src/themes/md/default.tokens.ts
Issue number: resolves internal
What is the current behavior?
Buttons only have bold colors. Unlike chip and badge, which also have a
colorprop, there's nohueprop for muted colors.What is the new behavior?
hueprop toion-button. It reads the prop first, thenIonButton.huein the theme config, and defaults tobold, like chiphue.subtleto the recipe in every theme, with the same fills, states,semanticcolors, ripple and disabled styles asboldcontrasttextforegroundtext and a subtleshadeborder. md keeps its neutral 12% borderforegroundtextforeground, and text on an opaque fill of the color usescontrast, matchingion-textand FW-6422foregroundfor their text too, like clear. In ios, clear buttons with acoloralso keepforegroundon hover and focusion-datetimeandion-input-password-togglestaybold, since those components only style the bold tokens, so a hue from the config doesn't restyle themDoes this introduce a breaking change?
The text of
outlinebuttons inmdandiosuses the color's darkerforegroundinstead ofbase. Inios, clear buttons with acoloralso keepforegroundon hover and focus. To revert, setIonButton.hue.bold.outline.<state>.color(.semantic.<state>.colorwith acolor) for global styles, or--ion-button-hue-bold-outline-<state>-colorfor component-specific styles, to the color'sbase. Foriosclearbuttons with acolor, setIonButton.hue.bold.clear.semantic.<state>.coloror--ion-button-hue-bold-clear-semantic-<state>-colorforhoverandfocusto the color'sbaseas well.Other information
Why these choices
#D9E5FCfor primary), measured from the kit and from a nativeUIButtonin its tinted configuration. The shared palette (#EBF1FD) matches ios chips and badges, works for everycolorand follows the palette in dark mode. iOS 17's value would make subtle buttons darker than the subtle chips beside themforegroundin md and ios: the text sits on the page, not on a fill of the color, so it follows the same rule as clear. Usingbasemade outline labels lighter than clear labels in the same colorKnown gaps
light,warningandsuccesstext fails WCAG's 4.5:1 on white, the same as in subtle chips. The colors come from the shared palette, so the fix belongs there (FW-6417). Subtlelightalso fails in dark mode. The axe check leaves these colors out until FW-6417 fixes themion-buttonsand toolbar migrations can add their ownHow to test