Skip to content

NAS-141479 / 27.0.0-BETA.1 / Migrate Sharing/Shared components slide-ins & modal-header to tn-* #13927

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
AlexKarpov98 merged 4 commits into from
Aug 18, 2026
Merged

NAS-141479 / 27.0.0-BETA.1 / Migrate Sharing/Shared components slide-ins & modal-header to tn-* #13927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
af871c9
NAS-141479: Migrate Sharing slide-ins & modal-header to tn-*
AlexKarpov98 Aug 13, 2026
df5d633
NAS-141479: Address review feedback on the Sharing tn-* migration
AlexKarpov98 Aug 13, 2026
2103f46
NAS-141479: Address second-round review feedback on the Sharing tn-* …
AlexKarpov98 Aug 14, 2026
9d921c3
Merge branch 'master' into NAS-141479
AlexKarpov98 Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/app/pages/sharing/components/shares-dashboard/iscsi-card/iscsi-card.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
IxTablePagerShowMoreComponent,
} from 'app/modules/ix-table/components/ix-table-pager-show-more/ix-table-pager-show-more.component';
import { FormSidePanelService } from 'app/modules/slide-ins/form-side-panel/form-side-panel.service';
import { SlideInRef } from 'app/modules/slide-ins/slide-in-ref';
import { SlideInResult } from 'app/modules/slide-ins/slide-in-result';
import { IscsiCardComponent } from 'app/pages/sharing/components/shares-dashboard/iscsi-card/iscsi-card.component';
import {
Expand Down Expand Up @@ -57,12 +56,6 @@ describe('IscsiCardComponent', () => {
},
] as IscsiTarget[];

const slideInRef: SlideInRef<undefined, unknown> = {
close: jest.fn(),
requireConfirmationWhen: jest.fn(),
getData: jest.fn((): undefined => undefined),
};

const createComponent = createComponentFactory({
component: IscsiCardComponent,
imports: [IxTablePagerShowMoreComponent,
Expand All @@ -79,7 +72,6 @@ describe('IscsiCardComponent', () => {
mockProvider(FormSidePanelService, {
open: jest.fn(() => SlideInResult.empty()),
}),
mockProvider(SlideInRef, slideInRef),
mockProvider(LicenseService, {
hasFibreChannel$: of(true),
}),
Expand Down
12 changes: 0 additions & 12 deletions src/app/pages/sharing/components/shares-dashboard/nfs-card/nfs-card.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import {
} from 'app/modules/ix-table/components/ix-table-pager-show-more/ix-table-pager-show-more.component';
import { LoaderService } from 'app/modules/loader/loader.service';
import { FormSidePanelService } from 'app/modules/slide-ins/form-side-panel/form-side-panel.service';
import { SlideIn } from 'app/modules/slide-ins/slide-in';
import { SlideInRef } from 'app/modules/slide-ins/slide-in-ref';
import { SlideInResult } from 'app/modules/slide-ins/slide-in-result';
import { SnackbarService } from 'app/modules/snackbar/services/snackbar.service';
import { ApiService } from 'app/modules/websocket/api.service';
Expand Down Expand Up @@ -64,12 +62,6 @@ describe('NfsCardComponent', () => {
},
] as NfsShare[];

const slideInRef: SlideInRef<NfsShare | undefined, unknown> = {
close: jest.fn(),
requireConfirmationWhen: jest.fn(),
getData: jest.fn((): undefined => undefined),
};

const commonImports = [IxTablePagerShowMoreComponent];

const commonProviders = [
Expand All @@ -78,7 +70,6 @@ describe('NfsCardComponent', () => {
confirm: jest.fn(() => of(true)),
confirmDelete: jest.fn(() => of(undefined)),
}),
mockProvider(SlideInRef, slideInRef),
mockProvider(TnDialog, {
open: jest.fn(() => ({
closed: of(true),
Expand All @@ -87,9 +78,6 @@ describe('NfsCardComponent', () => {
mockProvider(LoaderService, {
withLoader: jest.fn(() => (source$: unknown) => source$),
}),
mockProvider(SlideIn, {
open: jest.fn(() => SlideInResult.empty()),
}),
mockProvider(FormSidePanelService, {
open: jest.fn(() => SlideInResult.empty()),
}),
Expand Down
31 changes: 22 additions & 9 deletions ...ring/components/shares-dashboard/service-state-button/service-state-button.component.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,26 @@
<button
mat-stroked-button
<!-- A read-only status readout, not a control: it carries no action, so it is a labelled
`role="img"` region rather than the button Material rendered (no tn-* outlined pill carries the
green / grey / orange service colours). The role is deliberately not live: `service` resolves
asynchronously on page load and `service.query` events push state changed elsewhere into the
store, so a `role="status"` region would announce unprompted rather than only confirm an action
taken from this card. The label carries the same copy as the tooltip, which is pointer-only now
that the readout is not focusable. `tnTestIdType="button"` pins the legacy
`button-service-status-*` id that `[ixTest]` on the old `<button>` resolved to. -->
@let statusLabel = 'The {name} service is {status}' | translate: {
name: service()?.service | mapValue: serviceNames,
status: service()?.state | lowercase
};

<span
class="service-status"
role="img"
tnTestIdType="button"
[ngClass]="statusClass()"
[attr.aria-roledescription]="'Service status' | translate"
[ixTest]="['service-status', service()?.service]"
[tnTooltip]="'The {name} service is {status}' | translate: {
name: service()?.service | mapValue: serviceNames,
status: service()?.state | lowercase
}"
[attr.aria-label]="service() ? statusLabel : null"
Comment thread
AlexKarpov98 marked this conversation as resolved.
[tnTestId]="['service-status', service()?.service]"
[tnTooltip]="statusLabel"
[tnTooltipDisabled]="!service()"
[ngClass]="statusClass()"
>
{{ service()?.state | titlecase }}
</button>
</span>
21 changes: 21 additions & 0 deletions ...ring/components/shares-dashboard/service-state-button/service-state-button.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,24 @@
margin-left: 8px;
margin-right: 8px;
}

// Reproduces the outlined-pill look the Material stroked button + `.fn-theme-*` used to give
// this readout. `background: transparent` is required: the global `.fn-theme-*` helpers paint a
// filled background, and these badges want the theme colour on the text only.
.service-status {

Check notice on line 9 in src/app/pages/sharing/components/shares-dashboard/service-state-button/service-state-button.component.scss

View workflow job for this annotation

GitHub Actions / review / Automatic PR review 

LOW: New .service-status block re-implements the shared scss-imports/status-pill mixin shape verbatim instead of including it, so the mixin's documented removal follow-up will not sweep this copy
align-items: center;
background: transparent;
border: 1px solid currentColor;
border-radius: 25px;
display: inline-flex;
font-size: 14px;
font-weight: 700;
height: 30px;
line-height: 1;
padding: 0 16px;
white-space: nowrap;
Comment on lines +9 to +20

@github-actions github-actions Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW — The shape half of this block is a verbatim copy of the shared status-pill mixin in src/assets/styles/scss-imports/status-pill.scss: align-items, background(-color), border, border-radius, display, font-weight, height, white-space are identical, and only padding (0 16px vs 0 8px), line-height (1 vs initial) and the added font-size: 14px differ. That mixin's own comment says the shape lives there "rather than being copied into each cell's stylesheet", and it carries a documented removal plan ("See 'Migration follow-ups' in TRUENAS_UI_INTEGRATION.md — this goes away once the library ships a status pill") which a hand-rolled copy won't be swept by. With service-status-cell.component.scss already carrying a third variant, this is the fourth service/status pill in the tree.

Reusing it keeps only the part that genuinely differs — the fn-theme-* colour axis, which the mixin doesn't cover:

@import 'scss-imports/status-pill';

.service-status {
  @include status-pill;

  font-size: 14px;
  line-height: 1;
  padding: 0 16px;

  &.fn-theme-green { color: var(--green); }
  &.fn-theme-grey { color: var(--grey); }
  &.fn-theme-orange { color: var(--orange); }
}

Worth noting the existing tier-status-colors mixin (src/assets/styles/mixins/tier-status.scss) is not the right fit here even though it looks close — it maps .fn-theme-grey to var(--fg2), whereas the Material .mdc-button--outlined.fn-theme-grey override this replaces used var(--grey), so folding into it would shift the Stopped colour.


&.fn-theme-green { color: var(--green); }
&.fn-theme-grey { color: var(--grey); }
&.fn-theme-orange { color: var(--orange); }
}
38 changes: 21 additions & 17 deletions ...g/components/shares-dashboard/service-state-button/service-state-button.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
import { HarnessLoader } from '@angular/cdk/testing';
import { TestbedHarnessEnvironment } from '@angular/cdk/testing/testbed';
import { MatButtonHarness } from '@angular/material/button/testing';
import { createComponentFactory, Spectator } from '@ngneat/spectator/jest';
import { TnIconComponent, TnTooltipDirective } from '@truenas/ui-components';
import { MockComponent, MockDirective } from 'ng-mocks';
import { TnTooltipDirective } from '@truenas/ui-components';
import { MockDirective } from 'ng-mocks';
import { ServiceName } from 'app/enums/service-name.enum';
import { ServiceStatus } from 'app/enums/service-status.enum';
import { Service } from 'app/interfaces/service.interface';
Expand All @@ -12,37 +9,44 @@ import { ServiceStateButtonComponent } from 'app/pages/sharing/components/shares

describe('ServiceStateButtonComponent', () => {
let spectator: Spectator<ServiceStateButtonComponent>;
let loader: HarnessLoader;

const createComponent = createComponentFactory({
component: ServiceStateButtonComponent,
imports: [
MapValuePipe,
],
declarations: [
MockComponent(TnIconComponent),
MockDirective(TnTooltipDirective),
],
});

const getStatus = (): HTMLElement => spectator.query('.service-status') as HTMLElement;

beforeEach(() => {
spectator = createComponent();
loader = TestbedHarnessEnvironment.loader(spectator.fixture);
spectator.fixture.detectChanges();
});

it('shows service status based on service state', async () => {
it('shows service status based on service state', () => {
spectator.setInput('service', { id: 1, service: ServiceName.Nfs, state: ServiceStatus.Running } as Service);
spectator.setInput('count', 5);
const runningButton = await loader.getHarness(MatButtonHarness.with({ text: 'Running' }));
expect(runningButton).toExist();

expect(getStatus()).toHaveText('Running');
expect(getStatus()).toHaveClass('fn-theme-green');

spectator.setInput('service', { id: 1, service: ServiceName.Nfs, state: ServiceStatus.Stopped } as Service);
const stoppedButton = await loader.getHarness(MatButtonHarness.with({ text: 'Stopped' }));
expect(stoppedButton).toExist();
expect((await (await stoppedButton.host()).getAttribute('class'))?.split(' ').includes('fn-theme-grey')).toBe(true);

spectator.setInput('count', 0);
expect((await (await stoppedButton.host()).getAttribute('class'))?.split(' ').includes('fn-theme-grey')).toBe(true);
expect(getStatus()).toHaveText('Stopped');
expect(getStatus()).toHaveClass('fn-theme-grey');
});

it('exposes the status as a labelled, non-live region', () => {
spectator.setInput('service', { id: 1, service: ServiceName.Nfs, state: ServiceStatus.Running } as Service);

expect(getStatus()).toHaveAttribute('role', 'img');
expect(getStatus()).toHaveAttribute('aria-roledescription', 'Service status');
// The tooltip is pointer-only, so the same copy has to reach the accessibility tree.
expect(getStatus()).toHaveAttribute('aria-label', 'The NFS service is running');
// Preserved verbatim from the `<button [ixTest]>` this readout replaced.
expect(getStatus()).toHaveAttribute('data-test', 'button-service-status-nfs');
});
});
9 changes: 2 additions & 7 deletions ...haring/components/shares-dashboard/service-state-button/service-state-button.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,20 @@
import {
ChangeDetectionStrategy, Component, computed, input,
} from '@angular/core';
import { MatButton } from '@angular/material/button';
import { TranslateModule } from '@ngx-translate/core';
import { TnTooltipDirective } from '@truenas/ui-components';
import { TnTestIdDirective, TnTooltipDirective } from '@truenas/ui-components';
import { serviceNames } from 'app/enums/service-name.enum';
import { ServiceStatus } from 'app/enums/service-status.enum';
import { Service } from 'app/interfaces/service.interface';
import { MapValuePipe } from 'app/modules/pipes/map-value/map-value.pipe';
import { TestDirective } from 'app/modules/test-id/test.directive';

@Component({
selector: 'ix-service-state-button',
templateUrl: './service-state-button.component.html',
styleUrls: ['./service-state-button.component.scss'],
changeDetection: ChangeDetectionStrategy.OnPush,
imports: [
MatButton,
TestDirective,
TnTestIdDirective,
TnTooltipDirective,
NgClass,
TranslateModule,
Expand All @@ -27,11 +24,9 @@
TitleCasePipe,
],
})
export class ServiceStateButtonComponent {

Check notice on line 27 in src/app/pages/sharing/components/shares-dashboard/service-state-button/service-state-button.component.ts

View workflow job for this annotation

GitHub Actions / review / Automatic PR review 

LOW: Class, selector, directory and file names still say button although the template now renders an inert span with role=img and no click handler or focusability

@github-actions github-actions Bot Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW — After this change nothing in here is a button: the template renders an inert <span role="img"> with no click handler and no focusability, yet the class, the selector (ix-service-state-button), the directory and the file names all still say "button". The next reader who opens this looking for the control's action handler won't find one, and the two call sites (smb-list, webshare-list) read as if they're placing a control in the card header.

ix-service-status / ServiceStateComponent would match what it now is. The tnTestIdType="button" in the template is a separate, deliberate thing (it pins the legacy button-service-status-* ids) and should stay regardless — worth keeping the existing template comment so a rename doesn't invite "fixing" that too.

Non-blocking, and a rename touches a handful of files, so it's reasonable to defer — but the current names actively mislead.

readonly service = input<Service>();
readonly count = input<number>();

protected readonly serviceStatus = ServiceStatus;
protected readonly serviceNames = serviceNames;

readonly statusClass = computed(() => {
Expand Down
12 changes: 0 additions & 12 deletions src/app/pages/sharing/components/shares-dashboard/smb-card/smb-card.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import {
} from 'app/modules/ix-table/components/ix-table-pager-show-more/ix-table-pager-show-more.component';
import { LoaderService } from 'app/modules/loader/loader.service';
import { FormSidePanelService } from 'app/modules/slide-ins/form-side-panel/form-side-panel.service';
import { SlideIn } from 'app/modules/slide-ins/slide-in';
import { SlideInRef } from 'app/modules/slide-ins/slide-in-ref';
import { SlideInResult } from 'app/modules/slide-ins/slide-in-result';
import { SnackbarService } from 'app/modules/snackbar/services/snackbar.service';
import { ApiService } from 'app/modules/websocket/api.service';
Expand Down Expand Up @@ -62,12 +60,6 @@ describe('SmbCardComponent', () => {
},
] as SmbShare[];

const slideInRef: SlideInRef<SmbShare | undefined, unknown> = {
close: jest.fn(),
requireConfirmationWhen: jest.fn(),
getData: jest.fn((): undefined => undefined),
};

const commonImports = [IxTablePagerShowMoreComponent];

const commonProviders = [
Expand All @@ -76,7 +68,6 @@ describe('SmbCardComponent', () => {
confirm: jest.fn(() => of(true)),
confirmDelete: jest.fn(() => of(undefined)),
}),
mockProvider(SlideInRef, slideInRef),
mockProvider(TnDialog, {
open: jest.fn(() => ({
closed: of(true),
Expand All @@ -85,9 +76,6 @@ describe('SmbCardComponent', () => {
mockProvider(LoaderService, {
withLoader: jest.fn(() => (source$: unknown) => source$),
}),
mockProvider(SlideIn, {
open: jest.fn(() => SlideInResult.empty()),
}),
mockProvider(FormSidePanelService, {
open: jest.fn(() => SlideInResult.empty()),
}),
Expand Down
7 changes: 0 additions & 7 deletions ...p/pages/sharing/components/shares-dashboard/webshare-card/webshare-card.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
IxTablePagerShowMoreComponent,
} from 'app/modules/ix-table/components/ix-table-pager-show-more/ix-table-pager-show-more.component';
import { FormSidePanelService } from 'app/modules/slide-ins/form-side-panel/form-side-panel.service';
import { SlideIn } from 'app/modules/slide-ins/slide-in';
import { SlideInResult } from 'app/modules/slide-ins/slide-in-result';
import { SnackbarService } from 'app/modules/snackbar/services/snackbar.service';
import { TruenasConnectService } from 'app/modules/truenas-connect/services/truenas-connect.service';
Expand Down Expand Up @@ -91,9 +90,6 @@ describe('WebShareCardComponent', () => {
],
providers: [
mockAuth(),
mockProvider(SlideIn, {
open: jest.fn(() => SlideInResult.empty()),
}),
mockProvider(FormSidePanelService, {
open: jest.fn(() => SlideInResult.empty()),
}),
Expand Down Expand Up @@ -311,7 +307,6 @@ describe('WebShareCardComponent - TrueNAS Connect not configured', () => {
],
providers: [
mockAuth(),
mockProvider(SlideIn),
mockProvider(DialogService),
mockProvider(SnackbarService),
mockApi([
Expand Down Expand Up @@ -415,7 +410,6 @@ describe('WebShareCardComponent - No WebShare users configured', () => {
],
providers: [
mockAuth(),
mockProvider(SlideIn),
mockProvider(DialogService),
mockProvider(SnackbarService),
mockApi([
Expand Down Expand Up @@ -500,7 +494,6 @@ describe('WebShareCardComponent - TrueNAS Connect not configured but service run
],
providers: [
mockAuth(),
mockProvider(SlideIn),
mockProvider(DialogService),
mockProvider(SnackbarService),
mockApi([
Expand Down
Loading
Loading