fitzee commented on code in PR #45476:
URL: https://github.com/apache/superset/pull/45476#discussion_r4330828309
##########
superset-frontend/src/features/charts/ChartCard.test.tsx:
##########
@@ -167,7 +176,7 @@ test('with soft delete on, the card delete flow shows the
archive dialog', async
fireEvent.click(await screen.findByText('Archive'));
const dialog = await screen.findByRole('dialog');
- expect(within(dialog).getByText('Archive Sample
Chart?')).toBeInTheDocument();
+ expect(dialog).toHaveAccessibleName('Archive Sample Chart ?');
Review Comment:
nit: `'Archive Sample Chart ?'` (with a space before `?`) is
jsdom/dom-accessibility-api treating `<bdi>` as a block boundary. Browsers
compute the name as "Archive Sample Chart?", so the assertion pins what looks
like the defect being fixed. The `parentElement` `toHaveTextContent` check at
the end of the new RTL test is the one that guards punctuation placement.
Consider asserting text content here too (same at line 226).
##########
superset-frontend/src/utils/softDeleteCopy.ts:
##########
@@ -164,3 +166,83 @@ export function recoveredToast(
options: { allowHtml: true },
};
}
+
+type ArchivedObjectType = 'chart' | 'dashboard' | 'dataset';
+
+/** Build a collection URL beneath the configured application root. */
+function archiveHref(type: ArchivedObjectType): string {
+ return ensureAppRoot(`/archived/?type=${type}`);
+}
+
+/** Confirmation text with a direct route to the permitted archive collection.
*/
+export function archiveConfirmContent(
+ type: ArchivedObjectType,
+ typeLabel: string,
+ plural = false,
+): ReactElement {
+ return createElement(
+ Fragment,
+ null,
+ archiveConfirmDescription(typeLabel, plural),
+ ' ',
+ createElement('a', { href: archiveHref(type) }, t('Recently Archived')),
+ );
+}
+
+/** An escaped archive toast with a collection link; hard-delete copy stays
plain. */
+export function deletedToastContent(
+ name: string,
+ type: ArchivedObjectType,
+): ToastContent {
+ const text = deletedToast(name);
+ return isFeatureEnabled(FeatureFlag.SoftDelete)
+ ? {
+ text: `${escape(text)} <a
href="${escape(archiveHref(type))}">${escape(t('Recently Archived'))}</a>`,
+ options: { allowHtml: true },
+ }
+ : { text };
+}
+
+/** Isolate the asset name so its direction cannot move translated
punctuation. */
+export function archiveTitle(name: string): ReactElement {
+ const marker = '\uFFF0';
+ const [before, after] = t('Archive %(name)s?', { name: marker }).split(
+ marker,
+ );
+ return createElement(
+ Fragment,
+ null,
+ before,
+ createElement('bdi', { dir: 'auto' }, name),
+ after,
+ );
+}
+
+/** Explain dataset dependencies only when some dependent objects exist. */
+export function datasetArchiveImpact(
+ typeLabel: string,
+ chartCount: number,
+ dashboardCount: number,
+ plural = false,
+): string | null {
+ if (chartCount === 0 && dashboardCount === 0) return null;
Review Comment:
nit: the warning is skipped only when both counts are zero. A dataset whose
charts aren't on any dashboard still reads "linked to 2 charts on 0
dashboards", which is the zero-count wording this PR is meant to remove.
Consider a charts-only variant (e.g. "This %(type)s is used by %(charts)s.")
when `dashboardCount === 0`.
##########
superset-frontend/src/utils/softDeleteCopy.ts:
##########
@@ -164,3 +166,83 @@ export function recoveredToast(
options: { allowHtml: true },
};
}
+
+type ArchivedObjectType = 'chart' | 'dashboard' | 'dataset';
+
+/** Build a collection URL beneath the configured application root. */
+function archiveHref(type: ArchivedObjectType): string {
+ return ensureAppRoot(`/archived/?type=${type}`);
+}
+
+/** Confirmation text with a direct route to the permitted archive collection.
*/
+export function archiveConfirmContent(
+ type: ArchivedObjectType,
+ typeLabel: string,
+ plural = false,
+): ReactElement {
+ return createElement(
+ Fragment,
+ null,
+ archiveConfirmDescription(typeLabel, plural),
+ ' ',
+ createElement('a', { href: archiveHref(type) }, t('Recently Archived')),
Review Comment:
nit: this is a plain `<a>` inside the confirmation, before anything has been
archived. Clicking it does a full page load to Recently Archived with the
dialog never confirmed, so the item isn't archived and won't be in the list the
user just opened. Since the success toast now carries the same link, plain text
here (or `target="_blank"`) would avoid that dead end. The rendered copy also
repeats itself: "…moved to Recently Archived. You can recover it there.
Recently Archived".
##########
superset-frontend/src/features/charts/ChartCard.tsx:
##########
@@ -213,21 +218,25 @@ export default function ChartCard({
)
}
>
- <button
- type="button"
- css={menuItemButtonCss}
- data-test="chart-list-delete-option"
- className="action-button"
- onClick={allowEdit ? confirmDelete : undefined}
- >
- <Icons.DeleteOutlined
- iconSize="l"
- css={css`
- vertical-align: text-top;
- `}
- />{' '}
- {deleteActionLabel()}
- </button>
+ <span>
+ <MenuItemButton
+ type="button"
+
Review Comment:
nit: removing `css={menuItemButtonCss}` left blank lines inside the JSX
attribute lists here and at lines 131/155, and in DashboardCard.tsx at lines
121/142/174.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
