Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looks good overall, the icon-only sizes hold up across every size and theme! Just a couple of things, mainly wrapped text no longer naming the button for screen readers.
| // An icon-only button shows only its icon and badges with a `vertical` position | ||
| :host(.button-has-icon-only) ::slotted([slot="start"]), | ||
| :host(.button-has-icon-only) ::slotted([slot="end"]), | ||
| :host(.button-has-icon-only) ::slotted(:not([slot]):not(ion-badge[vertical])) { |
There was a problem hiding this comment.
This one worries me a bit, because text wrapped in an element loses its accessible name now. With <span>Add item</span> or <ion-label>Add item</ion-label> next to the icon, the button has an empty name, where before it was "Add item", and only bare text still works. A visually hidden span is a pretty common way to label an icon button, so could the default-slot elements be visually hidden instead of display: none? If not, BREAKING.md should say only bare text names the button, since right now it says hidden text still does. A test with wrapped text would catch this either way.
There was a problem hiding this comment.
Good catch. In a9d788d, content without a slot is visually hidden instead of display: none, so <span> and <ion-label> name the button again. BREAKING.md and the icon-only slot docs now say it's visually hidden. The test covers bare text, <span> and <ion-label>, and fails on the old code.
One side effect: an inline badge now adds to the name, the same way a vertical badge already did.
| } | ||
|
|
||
| // An icon-only button shows only its icon and badges with a `vertical` position | ||
| :host(.button-has-icon-only) ::slotted([slot="start"]), |
There was a problem hiding this comment.
Outside CSS that sets display beats these ::slotted() rules, so the start and end content can come back. In the ionic theme the global link style does it, so <a slot="start" href="#x">Link</a> in an icon-only button still renders, gets its own tab stop, and becomes the button's name. Could we skip rendering the start and end slots when hasIconOnly, or make these !important? An <a slot="start"> case in the tests would cover it.
There was a problem hiding this comment.
Went with skipping the slots in 22a4ad5. An icon-only button doesn't render its start and end slots, so their content isn't drawn, focused or used in the name, whatever outside CSS sets. The icon-only tests now run in the ionic theme with an <a slot="start">, and fail on the old code.
Issue number: resolves internal
What is the current behavior?
An icon-only button renders anything else placed in it: text, icons in the
startandendslots, and badges without averticalposition. That content breaks the square shape, as raised in the review of #31043.What is the new behavior?
icon-onlyslot shows only that icon and any badge with averticalpositionstartandendslots aren't rendered. Text and other content without a slot are visually hidden, and the button keeps its square size<span>or<ion-label>Does this introduce a breaking change?
A button with an icon in the
icon-onlyslot now shows only that icon and any badge with averticalposition. Text, elements in thestartandendslots, and other content are hidden. Text and elements without a slot are only visually hidden, so they still name the button for assistive technology. To show other content, remove theicon-onlyslot and use thestartorendslot for the icon.Other information