Skip to content

Commit 83612a6

Browse files
peterzimonpeterzimon
authored
Improved theme error modal visuals (TryGhost#30725)
The sidebar theme-error dialog displayed literal HTML tags and used different typography and layout from the theme-upload dialog. It now reuses the upload flow's issue list, including severity badges, code chips, issue counts, and expandable details, in a matching 600px-wide modal. Both dialogs now use medium-weight issue titles and an affected-files section with a medium-weight heading and primary text color. This is a presentation-only change using the existing theme data; the sidebar retains its OK action. The shared sidebar banner container is also centered using the sidebar width, correcting the theme-error panel's leftward offset while preserving its width. Refs https://linear.app/ghost/issue/DES-1481/improve-visuals-of-theme-with-errors-modal Validation: - Updated the existing sidebar modal acceptance test; no new tests added. - All 64 sidebar and theme-upload acceptance tests passed, along with Admin typechecking and repository formatting/lint checks. Final typography adjustments passed formatting checks. - Visually checked the sidebar dialog and expanded details in the running Admin. - Visually verified the centered sidebar banner; all three existing banner unit tests passed. - The full `pnpm check` run was stopped after failures in unrelated Ghost Core and Unsplash selector tests.
1 parent 7a744d9 commit 83612a6

5 files changed

Lines changed: 58 additions & 81 deletions

File tree

‎apps/admin/src/layout/app-sidebar/app-sidebar-banner.tsx‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ function AppSidebarBanner({ banner }: AppSidebarBannerProps) {
1414
return null;
1515
}
1616

17-
return <div className="fixed bottom-[92px] left-3 z-50 max-w-[276px]">{resolvedBanner}</div>;
17+
return (
18+
<div className="fixed bottom-[92px] left-[calc(var(--sidebar-width)/2)] z-50 w-[276px] -translate-x-1/2">
19+
{resolvedBanner}
20+
</div>
21+
);
1822
}
1923

2024
export default AppSidebarBanner;

‎apps/admin/src/layout/app-sidebar/theme-errors-dialog.tsx‎

Lines changed: 9 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useState } from 'react';
1+
import { ThemeValidationIssueList } from '@/settings/api';
22
import {
33
Button,
44
Dialog,
@@ -7,7 +7,6 @@ import {
77
DialogHeader,
88
DialogTitle,
99
} from '@tryghost/shade/components';
10-
import { LucideIcon } from '@tryghost/shade/utils';
1110
import type { ThemeProblem } from '@tryghost/admin-x-framework/api/themes';
1211

1312
interface ThemeErrorsDialogProps {
@@ -17,81 +16,22 @@ interface ThemeErrorsDialogProps {
1716
warnings: ThemeProblem<'warning'>[];
1817
}
1918

20-
function ThemeErrorItem({ error }: { error: ThemeProblem }) {
21-
const [expanded, setExpanded] = useState(false);
22-
23-
return (
24-
<li className="border-b border-border last:border-0">
25-
<button
26-
className="flex w-full items-center gap-2 py-3 text-left text-sm font-medium"
27-
type="button"
28-
onClick={() => setExpanded(!expanded)}
29-
>
30-
<LucideIcon.ChevronRight
31-
className={`size-4 shrink-0 text-muted-foreground transition-transform duration-200 ${expanded ? 'rotate-90' : ''}`}
32-
/>
33-
<span>{error.rule}</span>
34-
</button>
35-
{expanded && (
36-
<div className="pb-3 pl-6 text-sm text-muted-foreground">
37-
<p dangerouslySetInnerHTML={{ __html: error.details }} />
38-
{error.failures?.length > 0 && (
39-
<div className="mt-2">
40-
<h6 className="text-xs font-semibold text-muted-foreground uppercase">
41-
Affected files:
42-
</h6>
43-
<ul className="mt-1 list-disc pl-4">
44-
{error.failures.map((failure) => (
45-
<li key={`${failure.ref}-${failure.message || ''}`}>
46-
<code className="text-xs">{failure.ref}</code>
47-
{failure.message && <>: {failure.message}</>}
48-
</li>
49-
))}
50-
</ul>
51-
</div>
52-
)}
53-
</div>
54-
)}
55-
</li>
56-
);
57-
}
58-
5919
function ThemeErrorsDialog({ open, onOpenChange, errors, warnings }: ThemeErrorsDialogProps) {
6020
return (
6121
<Dialog open={open} onOpenChange={onOpenChange}>
62-
<DialogContent className="flex max-h-[85vh] max-w-lg flex-col">
22+
<DialogContent
23+
aria-describedby={undefined}
24+
className="flex max-h-[85vh] w-[calc(100%-2rem)] max-w-[600px] flex-col bg-background"
25+
>
6326
<DialogHeader>
64-
<DialogTitle className="text-2xl tracking-tighter">Theme errors</DialogTitle>
27+
<DialogTitle className="leading-normal tracking-normal">Theme errors</DialogTitle>
6528
</DialogHeader>
6629

67-
<section className="-mx-6 flex-1 overflow-y-auto px-6">
68-
{errors.length > 0 && (
69-
<div>
70-
<h2 className="mb-1 text-sm font-semibold">Errors</h2>
71-
<p className="mb-2 text-xs text-muted-foreground">
72-
Highly recommended to fix, functionality could be restricted
73-
</p>
74-
<ul className="border-t border-border">
75-
{errors.map((error) => (
76-
<ThemeErrorItem key={error.code} error={error} />
77-
))}
78-
</ul>
79-
</div>
80-
)}
81-
82-
{warnings.length > 0 && (
83-
<div className={errors.length > 0 ? 'mt-4' : ''}>
84-
<h2 className="mb-1 text-sm font-semibold">Warnings</h2>
85-
<ul className="border-t border-border">
86-
{warnings.map((warning) => (
87-
<ThemeErrorItem key={warning.code} error={warning} />
88-
))}
89-
</ul>
90-
</div>
91-
)}
30+
<section className="-mx-6 min-h-0 flex-1 overflow-y-auto px-6">
31+
<ThemeValidationIssueList problems={[...errors, ...warnings]} />
9232
</section>
9333

94-
<DialogFooter>
34+
<DialogFooter className="shrink-0">
9535
<Button onClick={() => onOpenChange(false)}>OK</Button>
9636
</DialogFooter>
9737
</DialogContent>

‎apps/admin/src/layout/sidebar.acceptance.test.tsx‎

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ describe('Network notification badge', () => {
359359
describe('Theme error notification', () => {
360360
const DEPRECATED_HELPER_ERROR = {
361361
code: 'GS001-DEPR-PURL',
362-
rule: 'Replace deprecated helper',
362+
rule: 'Replace deprecated <code>{{pageUrl}}</code> helper',
363363
details: 'The <code>{{pageUrl}}</code> helper has been deprecated.',
364364
failures: [{ ref: 'default.hbs', message: 'deprecated usage' }],
365365
fatal: false,
@@ -386,19 +386,51 @@ describe('Theme error notification', () => {
386386
await expect.element(sidebarScreen.themeErrorsBanner()).toBeVisible();
387387
});
388388

389-
it('opens the theme errors dialog when the banner is clicked', async () => {
389+
it('shows formatted theme issues and expandable details when the banner is clicked', async () => {
390390
await renderAdminApp('/site', {
391391
boot: {
392-
browseActiveTheme: { response: activeThemeResponse({ errors: [DEPRECATED_HELPER_ERROR] }) },
392+
browseActiveTheme: {
393+
response: activeThemeResponse({
394+
errors: [DEPRECATED_HELPER_ERROR],
395+
warnings: [
396+
{
397+
code: 'GS001-DEPR-TWITTER-URL',
398+
rule: 'Replace <code>{{twitter_url}}</code>',
399+
details: 'Use the social_url helper.',
400+
failures: [],
401+
fatal: false,
402+
level: 'warning',
403+
},
404+
],
405+
}),
406+
},
393407
},
394408
});
395409

396410
await sidebarScreen.themeErrorsBanner().click();
397411

398-
await expect.element(sidebarScreen.themeErrorsDialog()).toBeVisible();
399-
await expect
400-
.element(sidebarScreen.themeErrorsDialog())
401-
.toHaveTextContent('Replace deprecated helper');
412+
const dialog = sidebarScreen.themeErrorsDialog();
413+
await expect.element(dialog).toBeVisible();
414+
await expect.element(dialog).toHaveTextContent('1 error, 1 warning');
415+
await expect.element(dialog).toHaveTextContent('Replace deprecated {{pageUrl}} helper');
416+
await expect.element(dialog).not.toHaveTextContent('<code>');
417+
418+
const error = dialog.getByRole('button', { name: /GS001-DEPR-PURL/ });
419+
const warning = dialog.getByRole('button', { name: /GS001-DEPR-TWITTER-URL/ });
420+
await expect.element(error).toHaveAttribute('aria-expanded', 'false');
421+
await expect.element(warning).toHaveAttribute('aria-expanded', 'false');
422+
expect(error.element().querySelector('code')?.textContent).toBe('{{pageUrl}}');
423+
424+
await error.click();
425+
await expect.element(dialog).toHaveTextContent('The {{pageUrl}} helper has been deprecated.');
426+
await expect.element(dialog).toHaveTextContent('Affected files');
427+
await expect.element(dialog).toHaveTextContent('default.hbs: deprecated usage');
428+
await warning.click();
429+
await expect.element(dialog).toHaveTextContent('Use the social_url helper.');
430+
await expect.element(error).toHaveAttribute('aria-expanded', 'true');
431+
432+
await dialog.getByRole('button', { name: 'OK', exact: true }).click();
433+
await expect.element(dialog).not.toBeInTheDocument();
402434
});
403435

404436
it('shows no banner when the active theme has no errors', async () => {

‎apps/admin/src/settings/api.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55
export { settingsRouteChildren } from './routes';
66
export { canAccessSettingsRoute } from './settings-access';
7+
export { ThemeValidationIssueList } from './site/theme/theme-validation-details';
78

89
// Lazy entry, not a component re-export: the shell mounts it behind `lazy:`,
910
// so a static re-export would pull the chunk into the shell bundle.

‎apps/admin/src/settings/site/theme/theme-validation-details.tsx‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,10 @@ const CODE_CHIP =
2626
'[&_code]:rounded-md [&_code]:border-0 [&_code]:bg-secondary [&_code]:px-1 [&_code]:py-0.5 [&_code]:align-baseline [&_code]:font-mono [&_code]:text-sm [&_code]:text-inherit [&_code]:leading-[inherit] [&_code]:whitespace-nowrap';
2727

2828
/** gscan writes `rule` and `details` as HTML, rendered verbatim. */
29-
const RULE_HTML = `text-base leading-[1.55] font-semibold text-foreground ${CODE_CHIP}`;
29+
const RULE_HTML = `text-base leading-[1.55] font-medium text-foreground ${CODE_CHIP}`;
3030
const DETAILS_HTML = `text-base leading-[1.55] text-foreground [&_a]:underline ${CODE_CHIP}`;
3131

32-
const FAILURE_LIST = `space-y-1 text-base text-muted-foreground ${CODE_CHIP}`;
32+
const FAILURE_LIST = `space-y-1 text-base text-foreground ${CODE_CHIP}`;
3333

3434
function countBySeverity(problems: ThemeProblem[]) {
3535
return SEVERITY_ORDER.map((severity) => ({
@@ -67,7 +67,7 @@ function ProblemDetails({ problem }: { problem: ThemeProblem }) {
6767
<div dangerouslySetInnerHTML={{ __html: problem.details }} className={DETAILS_HTML} />
6868
{problem.failures?.length > 0 && (
6969
<div>
70-
<h6 className="mb-1 text-base font-semibold text-muted-foreground">Affected files</h6>
70+
<h6 className="mb-1 text-base font-medium text-foreground">Affected files</h6>
7171
<ul className={FAILURE_LIST}>
7272
{problem.failures.map((failure) => (
7373
<li key={`${failure.ref}-${failure.message || ''}`}>
@@ -126,7 +126,7 @@ function ValidationMessageRow({ errorLabel, message }: { errorLabel: string; mes
126126
<div className="flex items-center gap-3">
127127
<SeverityBadge variant="destructive">{errorLabel}</SeverityBadge>
128128
</div>
129-
<p className="text-base font-semibold text-foreground">{message}</p>
129+
<p className="text-base font-medium text-foreground">{message}</p>
130130
</div>
131131
);
132132
}

0 commit comments

Comments
 (0)