Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| // -------------------------------------------------- | ||
|
|
||
| .action-sheet-button-label { | ||
| gap: 12px; |
There was a problem hiding this comment.
I decided to use the same value that ionic uses.
| <div class="alert-button-inner"> | ||
| <div class="alert-checkbox-icon"> | ||
| <div class="alert-checkbox-inner"></div> | ||
| {inputs.map((i) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
| <div class="alert-button-inner"> | ||
| <div class="alert-radio-icon"> | ||
| <div class="alert-radio-inner"></div> | ||
| {inputs.map((i) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
| */ | ||
| this.closeModal(); | ||
| } | ||
| {this.options.map((option, index) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
| onIonChange={(ev) => { | ||
| this.setChecked(ev); | ||
| this.callOptionHandler(ev); | ||
| return this.options.map((option, index) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
| /** | ||
| * Text that is placed underneath the option text to provide additional details about the option. | ||
| */ | ||
| @Prop() description?: string; |
There was a problem hiding this comment.
Description isn't being used here to display on the screen because ion-select-option isn't used to display the options, it's done through the respective interface. So description is passed to the interface.
| <slot name="start"></slot> | ||
| <slot></slot> | ||
| <slot name="end"></slot> |
There was a problem hiding this comment.
Slots aren't being used here to display on the screen because ion-select-option isn't used to display the options, it's done through the respective interface. So slots are passed to the interface and the rendering is done with a new utility.
Even though it's not used to display here, it's still useful to include here so the docs can generate that "slots" are available for the options.
| onIonChange={(ev) => { | ||
| this.setChecked(ev); | ||
| this.callOptionHandler(ev); | ||
| return options.map((option, index) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
| */ | ||
| this.dismissParentPopover(); | ||
| } | ||
| {options.map((option, index) => { |
There was a problem hiding this comment.
The only change is the addition of optionLabelOptions and renderOptionLabel
ShaneK
left a comment
There was a problem hiding this comment.
This is looking really good! Just some feedback on some things I noticed
| // -------------------------------------------------- | ||
|
|
||
| .action-sheet-button-label { | ||
| gap: 12px; |
There was a problem hiding this comment.
Based it off ionic theme
ShaneK
left a comment
There was a problem hiding this comment.
Looks good to me! I consider these comments to be optional side quests (nits), feel free to disregard
|
|
||
| import { sanitizeDOMString } from './sanitization'; | ||
|
|
||
| interface RichContentOption { |
There was a problem hiding this comment.
RichContentOption is declared here with id, label, startContent, endContent, description, and a separate RichContentOption in select-interface.ts has only the three content fields. Same name, different shape, no shared source. Could we export one from select-interface.ts and import it here so we can avoid drift?
| <ion-select id="alert-select" label="Alert" placeholder="Select one" interface="alert"> | ||
| <ion-select-option value="full" description="Choose me!"> | ||
| <ion-badge slot="end">NEW</ion-badge> | ||
| <ion-avatar id="avatar" slot="start" size="large"> |
There was a problem hiding this comment.
id="avatar" is repeated 35+ times on this page (lines 60, 67, 74, etc.) Duplicate IDs are invalid HTML and none of the tests query by this id. Can we drop the attribute? I assume it's just copy pasta at this point
|
|
||
| // Render simple string label if there is no rich content to display | ||
| if (!hasRichContent && (typeof label === 'string' || !label)) { | ||
| // const Tag = useSpan ? 'span' : 'div'; |
There was a problem hiding this comment.
Dead commented line, Tag is already declared at line 84. Can you drop this?
Issue number: resolves #29890
What is the current behavior?
Select only allows plain text to be passed within it's options.
What is the new behavior?
ion-select-optioninnerHTMLTemplatesEnabledistruestartandendnamed slots for content that appears in the overlay interface but not in the selected textdescriptionprop for text rendered below the option label in the overlayion-selectstart/endslot contentaria-labelderives plain text only from the default slot, ensuring screen readers don't read slotted or element content--select-text-media-width,--select-text-media-height, etc.)Overlay interfaces (
alert,action-sheet,select-popover,select-modal)start,end,description, HTML label) consistently via the sharedrenderOptionLabelutilityActionSheetButton,AlertInput,SelectPopoverOption,SelectModalOption) are unchanged — rich content fields are on internal extended interfaces (SelectActionSheetButton,SelectAlertInput,SelectOverlayOption)sanitizeDOMStringbefore DOM injection to prevent XSSUtilities
select-option-render.tsx: shared render utility for option labels across all overlay componentsgetOptionContent: extracts and clones slot content fromion-select-optionfor overlays and selected textgetDefaultSlotPlainText— extracts plain text from the default slot, used for labels and ariaTests
Does this introduce a breaking change?
No, developers will be able to continue using plain text for select options as usual.
Other information
Preview