Fleet UI: Button icon padding correction - #50195
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #50195 +/- ##
=======================================
Coverage 68.21% 68.21%
=======================================
Files 3944 3945 +1
Lines 251263 251284 +21
Branches 13446 13453 +7
=======================================
+ Hits 171388 171404 +16
- Misses 64525 64531 +6
+ Partials 15350 15349 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Not ready to approve
The new icon rendering/styling doesn’t fully align with the stated padding-trim goal and alert-variant icons currently render with incorrect (dark) default colors on a red background.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates Fleet’s Button component to support standardized icon+label rendering via new leftIcon/rightIcon props, and migrates many call sites to use those props so icon spacing is controlled in one place (matching the design-system intent in #34377).
Changes:
- Added
leftIcon/rightIconprops tofrontend/components/buttons/Button/Button.tsx, including icon sizing by button size and new modifier classes. - Updated
frontend/components/buttons/Button/_styles.scssto add icon+label spacing rules and to fill in missingdefault__small/alert__smallstyling. - Migrated numerous UI call sites to replace inline
<Icon />usage inside<Button>withleftIcon/rightIcon.
File summaries
| File | Description |
|---|---|
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsx | Uses leftIcon for “Add package” button. |
| frontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsx | Converts badge buttons to leftIcon. |
| frontend/pages/SoftwarePage/SoftwareLibrary/SoftwareLibraryTable/SoftwareLibraryTable.tsx | Converts Categories control to leftIcon. |
| frontend/pages/SoftwarePage/SoftwareLibrary/SelfServiceCategoriesPage/SelfServiceCategoriesPage.tsx | Converts “Add category” to leftIcon. |
| frontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsx | Converts filter button to leftIcon. |
| frontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetMaintainedAppDetailsPage.tsx | Converts “Show details” to leftIcon. |
| frontend/pages/SoftwarePage/components/forms/AdvancedOptionsFields/AdvancedOptionsFields.tsx | Converts Schema action to rightIcon. |
| frontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsx | Converts “Edit” to leftIcon. |
| frontend/pages/queries/edit/components/QueryResults/QueryResults.tsx | Converts “Show query”/“Export” to rightIcon. |
| frontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsx | Converts Schema/Live report actions to rightIcon. |
| frontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsx | Converts Live report action to rightIcon. |
| frontend/pages/queries/details/components/QueryReport/QueryReport.tsx | Converts export action to rightIcon. |
| frontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsx | Converts “Add automations” to leftIcon. |
| frontend/pages/policies/edit/components/PolicyResults/PolicyResults.tsx | Converts “Show query” to rightIcon. |
| frontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsx | Converts Schema/Run policy actions to rightIcon. |
| frontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsx | Converts Run policy action to rightIcon. |
| frontend/pages/policies/details/components/PolicyAutomationsActivitiesTable/PolicyAutomationsActivitiesTable.tsx | Converts Reset policy action to rightIcon. |
| frontend/pages/policies/details/components/PolicyAutomationActivityDetailsModal/PolicyAutomationActivityDetailsModal.tsx | Converts Reset policy action to leftIcon. |
| frontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/GlobalVariables.tsx | Converts “Add variable” to leftIcon. |
| frontend/pages/ManageControlsPage/Scripts/components/ScriptListHeading/ScriptListHeading.tsx | Converts “Add script” to leftIcon. |
| frontend/pages/ManageControlsPage/components/UploadListHeading/UploadListHeading.tsx | Converts create action to leftIcon. |
| frontend/pages/labels/NewLabelPage/NewLabelPage.tsx | Converts Schema action to rightIcon. |
| frontend/pages/labels/components/DynamicLabelForm/DynamicLabelForm.tsx | Converts Schema action to rightIcon. |
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Converts multiple host actions to leftIcon. |
| frontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tsx | Converts resend/rotate actions to leftIcon. |
| frontend/pages/hosts/details/HostQueryReport/HQRTable/HQRTable.tsx | Converts “Show query”/“Export results” to rightIcon. |
| frontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsx | Converts Rotate password action to leftIcon. |
| frontend/pages/hosts/details/HostDetailsPage/modals/ManagedAccountModal/ManagedAccountModal.tsx | Converts Rotate password action to leftIcon. |
| frontend/pages/hosts/details/DeviceUserPage/components/InfoButton/InfoButton.tsx | Converts Info action to rightIcon. |
| frontend/pages/hosts/details/cards/User/User.tsx | Converts user actions to leftIcon/rightIcon. |
| frontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdateSoftwareItem/UpdateSoftwareItem.tsx | Converts installer action to leftIcon. |
| frontend/pages/hosts/details/cards/Software/HostSoftwareTable/HostSoftwareTable.tsx | Converts filter button to leftIcon. |
| frontend/pages/hosts/details/cards/Queries/HostQueries.tsx | Converts “Add report” to leftIcon. |
| frontend/pages/hosts/details/cards/HostSoftwareLibrary/HostSoftwareLibraryTable/HostSoftwareLibraryTable.tsx | Converts “Add software” to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraTenantsListHeader/EntraTenantsListHeader.tsx | Converts Add button to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListHeader/EntraClientIDsListHeader.tsx | Converts Add button to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MicrosoftEntraSection/WindowsAutomaticEnrollmentCard/WindowsEnrollmentCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/WindowsMdmCard/WindowsMdmCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/AppleMdmCard/AppleMdmCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/AndroidMdmCard/AndroidMdmCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AppleBusinessManagerSection/VppCard/VppCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AppleBusinessManagerSection/AppleAutomaticEnrollmentCard/AppleAutomaticEnrollmentCard.tsx | Converts Edit CTA to leftIcon. |
| frontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/components/OktaConditionalAccessModal/OktaConditionalAccessModal.tsx | Converts download action to rightIcon. |
| frontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CertAuthorityListHeader/CertAuthorityListHeader.tsx | Converts Add CA action to leftIcon. |
| frontend/components/queries/PackQueriesTable/PackQueriesTable.tsx | Converts “Add report” empty-state CTA to rightIcon. |
| frontend/components/Pagination/Pagination.tsx | Converts Previous button to leftIcon. |
| frontend/components/forms/LoginForm/LoginForm.tsx | Converts Back button to leftIcon. |
| frontend/components/FleetsDropdown/FleetsDropdown.tsx | Converts “Add fleet” to rightIcon. |
| frontend/components/EnrollSecrets/EnrollSecretModal/EnrollSecretModal.tsx | Converts “Add secret” actions to leftIcon. |
| frontend/components/buttons/Button/Button.tsx | Implements icon props, rendering, sizing, and modifier classes. |
| frontend/components/buttons/Button/Button.tests.tsx | Adds tests for new icon props/classes and icon-only behavior interaction. |
| frontend/components/buttons/Button/Button.stories.tsx | Updates stories to demonstrate leftIcon/rightIcon usage. |
| frontend/components/buttons/Button/_styles.scss | Adds icon-spacing modifiers and fills missing small styles for default/alert. |
| frontend/components/BackButton/BackButton.tsx | Converts back icon to leftIcon. |
| frontend/components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsx | Converts download actions to rightIcon. |
Review details
- Files reviewed: 55/55 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| // Default (green) buttons have white text — icons should match. | ||
| const iconColor = variant === "default" ? "core-fleet-white" : undefined; |
| &--with-left-icon, | ||
| &--with-right-icon { | ||
| padding-left: $pad-medium; | ||
| padding-right: $pad-medium; | ||
|
|
3dd5ad0 to
d16ef21
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
The new .button--with-left-icon/.button--with-right-icon SCSS rule is overly broad (affects unintended variants like link) and should be scoped to the intended variants before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
frontend/components/buttons/Button/Button.tsx:58
- The JSDoc for
leftIcon/rightIconsays the icon-side padding is "trimmed", which reads like asymmetric padding. This PR (and the new SCSS) implements symmetric horizontal padding + a gap, so the wording is likely misleading for future callers.
/** Renders an icon before the label with icon-side padding trimmed to match design. */
leftIcon?: IconNames;
/** Renders an icon after the label with icon-side padding trimmed to match design. */
rightIcon?: IconNames;
frontend/components/buttons/Button/_styles.scss:341
- The
button--with-left-icon/button--with-right-iconrule currently applies to any button variant (includinglink,pill,grey-pill, etc.), but the comment says it should apply only to variants that carry an icon (default/alert/secondary/subdued). As written, usingleftIcon/rightIconon e.g. alinkbutton would unexpectedly add 16px horizontal padding and change spacing.
// Buttons with leftIcon / rightIcon use uniform 16px horizontal padding and
// an 8px gap between icon and label at default size. Small buttons drop to
// 8px horizontal padding and a 4px gap. Applies to all variants that carry
// an icon (default, alert, secondary, subdued).
&--with-left-icon,
&--with-right-icon {
padding-left: $pad-medium;
padding-right: $pad-medium;
.children-wrapper {
gap: $pad-small;
}
}
- Files reviewed: 53/53 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The loading-state wrapper drops the children-wrapper class, which can cause size/layout shifts for icon+text buttons now using leftIcon/rightIcon.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
frontend/components/buttons/Button/Button.tsx:191
- When
isLoadingis true, the wrapper div uses onlytransparent-textand loses thechildren-wrapperclass, so the hidden content may lay out differently (e.g., block-level Icon wrappers stacking) and the button can change size while loading. Keepingchildren-wrapperapplied preserves flex layout/gap and avoids layout shift for icon+text buttons usingleftIcon/rightIcon.
<div className={isLoading ? "transparent-text" : "children-wrapper"}>
{leftIcon && (
<Icon name={leftIcon} size={iconSize} color={iconColor} />
)}
{children}
{rightIcon && (
<Icon name={rightIcon} size={iconSize} color={iconColor} />
)}
</div>
frontend/components/buttons/Button/Button.tsx:58
- The new
leftIcon/rightIconJSDoc says "icon-side padding trimmed", but the implementation keeps symmetric padding and only manages icon sizing/color + gap. Updating the comment avoids misleading future callers about padding behavior.
/** Renders an icon before the label with icon-side padding trimmed to match design. */
leftIcon?: IconNames;
/** Renders an icon after the label with icon-side padding trimmed to match design. */
rightIcon?: IconNames;
- Files reviewed: 53/53 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness and accessibility regressions (icon-only detection breaking legacy <Icon />-child usage and several new icon-only buttons missing ariaLabel) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (6)
frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/components/BootstrapPackageListItem/BootstrapPackageListItem.tsx:92
- This delete action is an icon-only button; it needs an
ariaLabel(or another accessible name) so assistive tech can identify it.
<Button
className={`${baseClass}__list-item-button`}
variant="subdued"
disabled={disabled}
onClick={() => onDelete(bootstrapPackage)}
icon="trash"
/>
frontend/components/buttons/Button/Button.tsx:183
- The new icon-only detection only works for the
iconprop. Existing icon-only buttons that render a lone<Icon />child (often to control color/size), like the close buttons infrontend/components/Modal/Modal.tsxandfrontend/components/AuthenticationNav/AuthenticationNav.tsx, will no longer getbutton--icon-onlyand will lose the square/centered styling. Also,iconis described in styles as a no-op for non-icon-enabled variants, but the current render logic will still insert the icon for any variant.
const hasLabel = React.Children.count(children) > 0;
// Square, centered layout for a bare icon on secondary/subdued — see #35329.
const isIconOnly =
!!icon && !hasLabel && (variant === "secondary" || variant === "subdued");
const hasIconWithLabel =
!!icon && hasLabel && ICON_ENABLED_VARIANTS.includes(variant!);
const fullClassName = classnames(
baseClass,
`${baseClass}--${variant}`,
className,
{
[`${baseClass}--${variant}__small`]: size === "small",
[`${baseClass}__wide`]: size === "wide",
[`${baseClass}--disabled`]: disabled,
[`${baseClass}--icon-only`]: isIconOnly,
[`${baseClass}--with-icon`]: hasIconWithLabel,
}
);
// Variants with white text (dark backgrounds) — icons and the loading
// spinner both render white so they're visible.
const hasWhiteText = WHITE_TEXT_VARIANTS.includes(variant!);
// Icons: 16px on default-size buttons, 12px on small-size buttons.
const iconSize = size === "small" ? "small" : "medium";
const iconColor = hasWhiteText ? "core-fleet-white" : undefined;
const iconElement = icon && (
<Icon name={icon} size={iconSize} color={iconColor} />
);
frontend/components/buttons/Button/Button.stories.tsx:44
Templateuseschildren: children ?? DEFAULT_ARGS.children, which treatsnullas “missing” and replaces it with the default label. That breaks theSecondaryIconOnlyVariant/SubduedIconOnlyVariantstories that intentionally passnullchildren to render an icon-only button.
const Template = (
variant: ButtonVariant,
children?: React.ReactNode,
extraArgs?: Partial<IButtonProps> // e.g. { size: "small" } or { disabled: true }
): Story => ({
args: {
...DEFAULT_ARGS,
variant,
children: children ?? DEFAULT_ARGS.children, // Fall back to default text
...extraArgs,
},
frontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/components/SetupAssistantProfileCard/SetupAssistantProfileCard.tsx:93
- These are icon-only buttons (no visible label). Without an
ariaLabel(or an equivalent accessible name), screen readers will announce them as unlabeled buttons.
<Button
className={`${baseClass}__download-button`}
variant="secondary"
onClick={onDownload}
icon="download"
/>
{!props.defaultProfile && (
<Button
className={`${baseClass}__delete-button`}
variant="secondary"
onClick={props.onDelete}
icon="trash"
/>
frontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/components/BootstrapPackageListItem/BootstrapPackageListItem.tsx:47
- This submit button is icon-only (no visible label). Add an
ariaLabelso it has an accessible name for screen readers.
This issue also appears on line 86 of the same file.
<Button
variant="subdued"
type="submit"
className={`${baseClass}__list-item-button`}
icon="download"
/>
frontend/pages/hosts/components/ScriptDetailsModal/ScriptDetailsModal.tsx:203
- This download action is now an icon-only button with no accessible name. Add an
ariaLabelso it’s announced correctly by screen readers.
- Files reviewed: 73/73 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness and accessibility issues to address (notably icon rendering on non-enabled variants and several icon-only buttons missing accessible labels), plus a Storybook template bug affecting icon-only stories.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (7)
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:225
- This delete action renders as an icon-only button but has no
ariaLabel/title, so it has no accessible name for screen readers.
<Button
disabled={disableChildren}
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickDelete(profile)}
icon="trash"
/>
frontend/components/buttons/Button/Button.tsx:192
iconis rendered for any variant, but icon styling is intended to be limited toICON_ENABLED_VARIANTS(per the constant name and the SCSS comment saying theiconprop is a no-op on link/pill/unstyled). As written,variant="pill"(and others) will still render the icon but without thebutton--with-iconclass/gap, and it contradicts the documented behavior.
const isIconOnly =
(variant === "secondary" || variant === "subdued") &&
((!!icon && !hasLabel) || hasLoneIconChild);
const hasIconWithLabel =
!!icon && hasLabel && ICON_ENABLED_VARIANTS.includes(variant!);
const fullClassName = classnames(
baseClass,
`${baseClass}--${variant}`,
className,
{
[`${baseClass}--${variant}__small`]: size === "small",
[`${baseClass}__wide`]: size === "wide",
[`${baseClass}--disabled`]: disabled,
[`${baseClass}--icon-only`]: isIconOnly,
[`${baseClass}--with-icon`]: hasIconWithLabel,
}
);
// Variants with white text (dark backgrounds) — icons and the loading
// spinner both render white so they're visible.
const hasWhiteText = WHITE_TEXT_VARIANTS.includes(variant!);
// Icons: 16px on default-size buttons, 12px on small-size buttons.
const iconSize = size === "small" ? "small" : "medium";
const iconColor = hasWhiteText ? "core-fleet-white" : undefined;
const iconElement = icon && (
<Icon name={icon} size={iconSize} color={iconColor} />
);
frontend/components/buttons/Button/Button.stories.tsx:43
Templateuseschildren ?? DEFAULT_ARGS.children, but the icon-only stories passchildren=nullexpecting to suppress the label. With??,nullfalls back to the default label, soSecondaryIconOnlyVariant/SubduedIconOnlyVariantwon’t actually be icon-only.
...DEFAULT_ARGS,
variant,
children: children ?? DEFAULT_ARGS.children, // Fall back to default text
...extraArgs,
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:197
- This is an icon-only button (no children) but it does not provide an accessible name (
ariaLabelortitle). Icon-only controls should have anariaLabelso screen readers can announce the action.
This issue also appears on line 219 of the same file.
<Button
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickInfo(profile)}
icon="info"
/>
frontend/components/TargetsInput/TargetsInputHostsTableConfig.tsx:32
- This is an icon-only remove button with no accessible name. Add an
ariaLabel(ortitle) so assistive tech can announce what the control does.
Header: "",
Cell: (cellProps: ITableStringCellProps) => (
<Button
onClick={() => handleRowRemove(cellProps.row)}
variant="subdued"
icon="close-filled"
/>
frontend/pages/labels/components/ManualLabelForm/LabelHostTargetTableConfig.tsx:30
- This icon-only remove button has no
ariaLabel/title, so it lacks an accessible name.
frontend/pages/admin/ManageUsersPage/components/ApiEndpointSelectorTable/ApiEndpointSelectorTable.tsx:128 - This icon-only remove button lacks an accessible name. Add an
ariaLabel(ortitle) so screen readers can identify the action.
- Files reviewed: 73/73 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Some icon-only buttons are missing accessible labels, and Button’s new spacing behavior doesn’t apply consistently to legacy <Icon>+label child patterns still used in the codebase.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:225
- This button is icon-only (no children), but it’s missing
ariaLabel, so it won’t be announced meaningfully to screen readers. Please add anariaLabel(e.g.Delete …) like other icon-only action buttons.
<Button
disabled={disableChildren}
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickDelete(profile)}
icon="trash"
/>
frontend/components/buttons/Button/Button.tsx:171
button--with-iconis only applied when using the newiconprop, so legacy<Icon>child patterns with a text label (still used in the codebase for custom icon colors/classes) won’t get the small-size 4px gap override in_styles.scssand will keep the default 8px gap. Consider detecting the legacy "icon + label" child pattern and applyingbutton--with-iconfor it as well so spacing stays consistent regardless of API.
const hasIconWithLabel =
!!icon && hasLabel && ICON_ENABLED_VARIANTS.includes(variant!);
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:197
- This button is icon-only (no children), but it’s missing
ariaLabel, so screen readers will announce it as an unlabeled button. Please add anariaLabeldescribing the action (consistent with the edit/download buttons below).
This issue also appears on line 219 of the same file.
<Button
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickInfo(profile)}
icon="info"
/>
- Files reviewed: 74/74 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Some newly icon-only action buttons are missing ariaLabel, leaving them without an accessible name.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:225
- This delete action button is icon-only but currently has no ariaLabel. Add an ariaLabel so the control is accessible and its purpose is announced correctly by assistive tech.
<Button
disabled={disableChildren}
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickDelete(profile)}
icon="trash"
/>
frontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsx:197
- This button is now icon-only (no children). Please add an ariaLabel so it has an accessible name for screen readers and keyboard users.
This issue also appears on line 219 of the same file.
<Button
className={`${subClass}__action-button`}
variant="secondary"
onClick={() => onClickInfo(profile)}
icon="info"
/>
- Files reviewed: 74/74 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
a5827a4 to
6ef525a
Compare
- Remove unused --success variant and --justify-left/--justify-right offset classes from Button/_styles.scss (no callers, not in ButtonVariant). - Trim the width-override selector in _global.scss to real classes (__wide, --icon-only); drop --icon / --icon__small which are never generated. - Drop dark-mode card/modal hover selectors for --inverse, --inverse-alert, --text-icon, --icon (variants don't exist); keep --secondary / --subdued.
There was a problem hiding this comment.
🟡 Human review recommended
It updates a core shared UI component and migrates many call sites, so a final human review/UX regression pass is needed to confirm no visual or accessibility regressions.
Review details
- Files reviewed: 80/80 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The Button icon-only path still allows unlabeled icon-only buttons (accessibility), and test coverage doesn’t yet assert the new white-text icon coloring for alert/oversized variants.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
frontend/components/buttons/Button/Button.tsx:220
- Icon-only buttons rely on
ariaLabelfor an accessible name, butaria-labelis currently set only fromariaLabel. If a caller forgetsariaLabelbut does settitle, screen readers will still have no label. Consider falling back totitlewhen the button is icon-only to avoid silent accessibility regressions.
aria-haspopup={ariaHasPopup}
aria-expanded={ariaExpanded}
aria-label={ariaLabel}
aria-pressed={ariaPressed}
frontend/components/buttons/Button/Button.tests.tsx:137
- The icon auto-coloring logic now treats
alertandoversizedas white-text variants (viaWHITE_TEXT_VARIANTS), but the tests only assert the white icon color forvariant="default". Adding analert(and/oroversized) assertion would prevent regressions like the one previously reported for alert buttons.
it("auto-colors the icon white on white-text variants (default)", () => {
render(
<Button variant="default" icon="plus">
Add
</Button>
);
expect(
screen
.getByTestId("plus-icon")
.querySelector("path")
?.getAttribute("stroke")
).toContain("core-fleet-white");
});
- Files reviewed: 80/80 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| ariaLabel, | ||
| ariaPressed, | ||
| size, | ||
| icon, |
There was a problem hiding this comment.
long awaited refactor into a prop
The prop enforces consistency that the child pattern couldn't:
- Spacing — Button owns the 8px/4px gap via .button--with-icon. As a child, every caller wrapped the label in (or forgot) and drift
accumulated.- Sizing — 16px on default, 12px on small, derived from size. As a child, callers had to pass size="small" on Icon separately. Migration
audit found ~24 sites where the child Icon was size="small" on a default-size button — a visual bug.- Color — auto-mapped from variant (white on dark-fill, ui-fleet-black-75 on secondary/subdued). As a child, callers had to remember the
right color per variant. Multiple bugs were fixed by this PR alone (e.g., alert button rendering a dark icon that vanished into the red fill).- Icon-only detection — with the prop + no children, Button knows to apply square 36/28px styling. As a child, the same could mean
"icon-only" or "icon that happens to be alone right now" depending on siblings.- Type safety — icon: IconNames is a constrained union. A child compiles fine.
- Non-icon variants no-op cleanly — a link or pill variant with icon="foo" drops the render (per our recent change). A child would
render unspaced garbage.Escape hatch preserved: the legacy -child pattern still triggers icon-only square styling. So sites that legitimately need a
non-standard color or className on the icon (rare — mostly TargetOption's green plus, FeedListItem etc.) can keep the child pattern.Short version: "same visual output, less caller responsibility." Every property the icon needs to look right (size, color, gap, position) can
be derived from the Button's own props. Making the caller specify them again invites drift.
- Console.warn (dev only) when an icon-only Button has neither ariaLabel nor title, so unlabeled icon-only buttons surface at runtime instead of shipping as a11y regressions. Browsers already fall back to title for the accessible name, so either prop satisfies a screen reader. - Clarify WHITE_TEXT_VARIANTS comment: oversized is listed for the loading spinner, not icons (not in ICON_ENABLED_VARIANTS). - Add tests: alert renders icon in white; oversized drops the icon; the new dev warning fires on missing labels but not when title is present.
| right: -$pad-small; | ||
| margin-left: -$pad-small; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 Ready to approve
Only a minor maintainability nit was found (dev warning could be more actionable for legacy icon-only buttons), and the functional/icon API changes appear consistent and well-covered by updated tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
frontend/components/buttons/Button/Button.tsx:224
- The dev-only accessibility warning will log
icon="unknown"for legacy icon-only buttons that use the<Icon />child pattern (sinceiconprop is unset). That makes it harder to locate the offending button in large pages; consider deriving the icon name from the lone<Icon>child when present.
console.warn(
`Icon-only Button (icon="${
icon ?? "unknown"
}") has no ariaLabel or title — ` +
`screen readers will announce it as an unlabeled button.`
- Files reviewed: 80/80 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
WalkthroughThe shared Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/buttons/Button/_styles.scss (1)
294-309: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the 16px horizontal padding override to
&--subdued.
button-variant(..., $inverse: true)applies the 8px padding mixin, and&--secondaryoverrides it withbutton-pad-8px-16px;&--subdueddoes not. Add that override to keepsubduedaligned with the default/alert/secondary full-size padding spec.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/buttons/Button/_styles.scss` around lines 294 - 309, Update the &--subdued style to apply the button-pad-8px-16px horizontal padding override after button-variant, matching the padding behavior of the default, alert, and secondary full-size button variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/components/buttons/Button/Button.tsx`:
- Around line 156-176: Update the accessibility warning condition near the
Button render logic to warn whenever an icon is rendered without a label on any
variant in ICON_ENABLED_VARIANTS, including default and alert, while preserving
the existing ariaLabel and title exemptions. Add or update Button.tests.tsx
coverage for an unlabeled default or alert icon button.
In `@frontend/pages/admin/OrgSettingsPage/cards/Info/Info.tsx`:
- Around line 92-94: Update the light- and dark-mode action buttons in LogoCard
to include the respective mode in each ariaLabel, making both “Replace” and
“Remove” actions uniquely identify whether they target the light or dark logo.
---
Outside diff comments:
In `@frontend/components/buttons/Button/_styles.scss`:
- Around line 294-309: Update the &--subdued style to apply the
button-pad-8px-16px horizontal padding override after button-variant, matching
the padding behavior of the default, alert, and secondary full-size button
variants.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 23aa501d-5578-4d51-b1f9-bad7335dce9d
📒 Files selected for processing (80)
frontend/components/AddHostsModal/PlatformWrapper/PlatformWrapper.tsxfrontend/components/BackButton/BackButton.tsxfrontend/components/EnrollSecrets/EnrollSecretModal/EnrollSecretModal.tsxfrontend/components/EnrollSecrets/EnrollSecretTable/EnrollSecretRow/EnrollSecretRow.tsxfrontend/components/FeedListItem/FeedListItem.tsxfrontend/components/FleetsDropdown/FleetsDropdown.tsxfrontend/components/Pagination/Pagination.tsxfrontend/components/Pagination/_styles.scssfrontend/components/TargetsInput/TargetsInputHostsTableConfig.tsxfrontend/components/buttons/Button/Button.stories.tsxfrontend/components/buttons/Button/Button.tests.tsxfrontend/components/buttons/Button/Button.tsxfrontend/components/buttons/Button/_styles.scssfrontend/components/forms/LoginForm/LoginForm.tsxfrontend/components/forms/fields/InputField/InputField.tsxfrontend/components/queries/PackQueriesTable/PackQueriesTable.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/Certificates/Certificates.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/ConfigurationProfiles.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/AssetListItem/AssetListItem.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/AssetsTab/AssetsTab.tsxfrontend/pages/ManageControlsPage/OSSettings/cards/ConfigurationProfiles/components/ProfileListItem/ProfileListItem.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsxfrontend/pages/ManageControlsPage/Scripts/components/ScriptListItem/ScriptListItem.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/BootstrapPackage/components/BootstrapPackageListItem/BootstrapPackageListItem.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/RunScript/components/SetupExperienceScriptCard/SetupExperienceScriptCard.tsxfrontend/pages/ManageControlsPage/SetupExperience/cards/SetupAssistant/components/SetupAssistantProfileCard/SetupAssistantProfileCard.tsxfrontend/pages/ManageControlsPage/Variables/cards/CustomHostVitalsTab/CustomHostVitalsTab.tsxfrontend/pages/ManageControlsPage/Variables/cards/GlobalVariables/GlobalVariables.tsxfrontend/pages/SoftwarePage/SoftwareAddPage/SoftwareFleetMaintained/FleetMaintainedAppDetailsPage/FleetMaintainedAppDetailsPage.tsxfrontend/pages/SoftwarePage/SoftwareInventory/SoftwareInventoryTable/SoftwareInventoryTable.tsxfrontend/pages/SoftwarePage/SoftwareLibrary/SelfServiceCategoriesPage/SelfServiceCategoriesPage.tsxfrontend/pages/SoftwarePage/SoftwareLibrary/SoftwareLibraryTable/SoftwareLibraryTable.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/LibraryItemAccordion/LibraryItemAccordion.tsxfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/SoftwareTitleDetailsPage.tsxfrontend/pages/SoftwarePage/components/cards/SoftwareDetailsSummary/SoftwareDetailsSummary.tsxfrontend/pages/SoftwarePage/components/forms/AdvancedOptionsFields/AdvancedOptionsFields.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CertAuthorityListHeader/CertAuthorityListHeader.tsxfrontend/pages/admin/IntegrationsPage/cards/CertificateAuthorities/components/CertAuthorityListItem/CertAuthorityListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/ConditionalAccess.tsxfrontend/pages/admin/IntegrationsPage/cards/ConditionalAccess/components/OktaConditionalAccessModal/OktaConditionalAccessModal.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListHeader/EntraClientIDsListHeader.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraClientIDsListItem/EntraClientIDsListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraTenantsListHeader/EntraTenantsListHeader.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/WindowsAutomaticEnrollmentPage/EntraTenantsListItem/EntraTenantsListItem.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AppleBusinessManagerSection/AppleAutomaticEnrollmentCard/AppleAutomaticEnrollmentCard.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/AppleBusinessManagerSection/VppCard/VppCard.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/AndroidMdmCard/AndroidMdmCard.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/AppleMdmCard/AppleMdmCard.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MdmSettingsSection/WindowsMdmCard/WindowsMdmCard.tsxfrontend/pages/admin/IntegrationsPage/cards/MdmSettings/components/MicrosoftEntraSection/WindowsAutomaticEnrollmentCard/WindowsEnrollmentCard.tsxfrontend/pages/admin/ManageUsersPage/components/ApiEndpointSelectorTable/ApiEndpointSelectorTable.tsxfrontend/pages/admin/OrgSettingsPage/cards/Info/Info.tsxfrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/hosts/ManageHostsPage/components/CustomLabelGroupHeading/CustomLabelGroupHeading.tsxfrontend/pages/hosts/ManageHostsPage/components/HostsFilterBlock/HostsFilterBlock.tsxfrontend/pages/hosts/components/ScriptDetailsModal/ScriptDetailsModal.tsxfrontend/pages/hosts/details/DeviceUserPage/components/InfoButton/InfoButton.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/ManagedAccountModal/ManagedAccountModal.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/RecoveryLockPasswordModal/RecoveryLockPasswordModal.tsxfrontend/pages/hosts/details/HostQueryReport/HQRTable/HQRTable.tsxfrontend/pages/hosts/details/OSSettingsModal/OSSettingsTable/OSSettingsResendCell/OSSettingsResendCell.tsxfrontend/pages/hosts/details/cards/HostSoftwareLibrary/HostSoftwareLibraryTable/HostSoftwareLibraryTable.tsxfrontend/pages/hosts/details/cards/Queries/HostQueries.tsxfrontend/pages/hosts/details/cards/Software/HostSoftwareTable/HostSoftwareTable.tsxfrontend/pages/hosts/details/cards/Software/SelfService/components/UpdatesCard/UpdateSoftwareItem/UpdateSoftwareItem.tsxfrontend/pages/hosts/details/cards/User/User.tsxfrontend/pages/labels/NewLabelPage/NewLabelPage.tsxfrontend/pages/labels/components/DynamicLabelForm/DynamicLabelForm.tsxfrontend/pages/labels/components/ManualLabelForm/LabelHostTargetTableConfig.tsxfrontend/pages/policies/details/PolicyDetailsPage/PolicyDetailsPage.tsxfrontend/pages/policies/details/components/PolicyAutomationActivityDetailsModal/PolicyAutomationActivityDetailsModal.tsxfrontend/pages/policies/details/components/PolicyAutomationsActivitiesTable/PolicyAutomationsActivitiesTable.tsxfrontend/pages/policies/edit/components/PolicyForm/PolicyForm.tsxfrontend/pages/policies/edit/components/PolicyResults/PolicyResults.tsxfrontend/pages/policies/edit/components/SaveNewPolicyModal/SaveNewPolicyModal.tsxfrontend/pages/queries/details/QueryDetailsPage/QueryDetailsPage.tsxfrontend/pages/queries/details/components/QueryReport/QueryReport.tsxfrontend/pages/queries/edit/components/EditQueryForm/EditQueryForm.tsxfrontend/pages/queries/edit/components/QueryResults/QueryResults.tsxfrontend/styles/global/_global.scss
| icon, | ||
| iconPosition, | ||
| } = this.props; | ||
| // The bordered "secondary" and borderless "subdued" variants render as a | ||
| // square when their only content is an icon (no text label) — see #35329. | ||
| // toArray strips false/null and flattens fragments, so we reliably detect a | ||
| // lone <Icon> child while ignoring conditional or wrapped text content. | ||
| // Square, centered layout for a bare icon on secondary/subdued — see #35329. | ||
| // Detects both the modern `icon` prop and the legacy `<Icon>` child pattern | ||
| // used by call sites that need a color/className override on the icon. | ||
| // `toArray` (not `Children.count`) so `{cond && "text"}` with a false `cond` | ||
| // reads as "no label" — otherwise the false child still counts as a label | ||
| // and the visually-lone icon skips its square styling. | ||
| const childArray = React.Children.toArray(children); | ||
| const isIconOnly = | ||
| (variant === "secondary" || variant === "subdued") && | ||
| const hasLabel = childArray.length > 0; | ||
| const hasLoneIconChild = | ||
| !icon && | ||
| childArray.length === 1 && | ||
| React.isValidElement(childArray[0]) && | ||
| childArray[0].type === Icon; | ||
| const isIconOnly = | ||
| (variant === "secondary" || variant === "subdued") && | ||
| ((!!icon && !hasLabel) || hasLoneIconChild); | ||
| const hasIconWithLabel = | ||
| !!icon && hasLabel && ICON_ENABLED_VARIANTS.includes(variant!); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Extend the dev accessibility warning to cover icon-only default/alert buttons.
ICON_ENABLED_VARIANTS includes default and alert, so icon renders on those variants even without children (Line 205). But isIconOnly at Line 172 only becomes true for secondary/subdued, and the warning at Line 211 fires only isIconOnly && !ariaLabel && !title. A <Button variant="default" icon="trash" /> with no ariaLabel or title renders an icon with no accessible name and produces no console warning, unlike the equivalent secondary/subdued case. Button.tests.tsx has no test for this scenario, so this path is unverified.
Broaden the warning condition to also catch icon-without-label on any icon-enabled variant, not only the styled icon-only ones.
🩹 Proposed fix
if (
process.env.NODE_ENV !== "production" &&
- isIconOnly &&
+ (isIconOnly ||
+ (!!icon && !hasLabel && ICON_ENABLED_VARIANTS.includes(variant!))) &&
!ariaLabel &&
!title
) {Also applies to: 186-224
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/components/buttons/Button/Button.tsx` around lines 156 - 176, Update
the accessibility warning condition near the Button render logic to warn
whenever an icon is rendered without a label on any variant in
ICON_ENABLED_VARIANTS, including default and alert, while preserving the
existing ariaLabel and title exemptions. Add or update Button.tests.tsx coverage
for an unlabeled default or alert icon button.
| icon="pencil" | ||
| ariaLabel="Replace logo" | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the logo action names unique per mode.
LogoCard renders separate light-mode and dark-mode actions, but both buttons expose Replace logo and Remove logo. The card label is not associated with the button. A screen reader user cannot identify which logo the action targets.
Use mode in each ariaLabel.
Proposed fix
- ariaLabel="Replace logo"
+ ariaLabel={`Replace ${mode} mode logo`}
...
- ariaLabel="Remove logo"
+ ariaLabel={`Remove ${mode} mode logo`}Also applies to: 106-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/admin/OrgSettingsPage/cards/Info/Info.tsx` around lines 92 -
94, Update the light- and dark-mode action buttons in LogoCard to include the
respective mode in each ariaLabel, making both “Replace” and “Remove” actions
uniquely identify whether they target the light or dark logo.
Migrate the toast's chevron and close buttons off the child <Icon> pattern to the new icon prop from #50195. Drop the explicit ui-fleet-black-75 color on close (subdued's default matches) and move the chevron rotation className from the Icon wrapper up to the Button so the existing __chevron / __chevron--open selectors still resolve. Also add two tests covering aria-controls on Button — attribute present when ariaControls is provided, omitted when undefined.
Issue
Closes #34377
Description
Buttonicon API: singleicon?: IconNamesprop with optionaliconPosition: "left" | "right"(default"left"). Icon-only buttons are derived fromicon && no children— no moreleftIcon/rightIcon/isIconOnlyheuristic and noReact.cloneElementacrobatics on icon children.Button: size derives from buttonsize(16px default, 12px small), color derives from variant (core-fleet-whiteondefault/alert/oversized; Icon default elsewhere). Callers stop passingsizeandcoloron icons.default,alert,secondary,subdued.--with-iconclass replaces--with-left-icon/--with-right-icon. Gap rules are scoped to the four icon-enabled variants, so theiconprop is a no-op onlink,pill,unstyled, etc.Button.tsxare the single source of truth for future variant tweaks:WHITE_TEXT_VARIANTSandICON_ENABLED_VARIANTS.leftIcon/rightIconprops and off the ad-hoc<Button><Icon .../></Button>child pattern. Sites that need a non-default icon color (e.g. TargetOption's green plus) or aclassNameon the Icon (e.g. FeedListItem's close) intentionally stay on the child pattern.variant="default" size="small"andvariant="alert" size="small"SCSS (no existing callers used those combos).Screenrecording
Testing
Summary by CodeRabbit