Repository navigation
fix(soft-delete): archive toast, counts, history and keyboard follow-ups - #45747
mikebridge wants to merge 30 commits into
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
i18n
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45747 +/- ##
==========================================
+ Coverage 85.00% 85.02% +0.01%
==========================================
Files 3080 3080
Lines 209266 209480 +214
Branches 49296 49328 +32
==========================================
+ Hits 177895 178106 +211
- Misses 27989 27995 +6
+ Partials 3382 3379 -3
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:
|
github-actions
Bot
added
the
requires:rebase
github-actions
Bot
removed
the
requires:rebase
|
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
Two of the described behaviours don't hold in a browser; details inline.
Nit: With SOFT_DELETE off, the delete warnings in DatasetList/index.tsx (~1621, ~1806) still say charts "on" dashboards; that file is outside this diff, so not inline.
| if (!isEqual(initialSortBy, sortBy)) { | ||
| setSortBy(initialSortBy); | ||
| } |
There was a problem hiding this comment.
Back/Forward now restores sortBy here, but CardSortSelect seeds its value once in useState, so card view keeps showing the old sort while cards use the restored one. Keying it, e.g. key={JSON.stringify(sortBy)} on both CardSortSelect uses in ListView.tsx, fixes it.
There was a problem hiding this comment.
Remaining: table view. The card Sort control now follows Back/Forward, but going Back from age desc to name asc refetches rows by Name while the Age header keeps its descending arrow (TableCollection seeds antd's header sort via defaultSortOrder). Recently Archived is table-only.
There was a problem hiding this comment.
Good catch. Both card-sort selectors are now keyed by the restored sort in a78cdbf. The desktop and mobile-drawer regressions check the selected option as well as the fetch sort after Back and Forward.
There was a problem hiding this comment.
Still open at 8d6f261: in table view, going Back from age desc to name asc refetches rows by Name while the Age header keeps aria-sort="descending". TableCollection passes only antd's defaultSortOrder, so the header never follows a restored sort.
Address the card-sort and keyboard-hint review threads on apache#45747: remount both sort selectors when URL state changes, and show permission explanations on the enabled menu trigger instead of unreachable disabled-item tab stops. Pin restored archive rows and describe dataset chart/dashboard counts independently in permanent-delete warnings.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
One new keyboard-focus regression in the shared card menu; details inline.
Nit: DatabaseList/index.tsx:1302 still says charts "that appear on" dashboards, unlike the reworded dataset copy; that file is outside this diff, so not inline.
|
On the review-body note about DatasetList: the single and bulk permanent-delete warnings now say charts "and" dashboards in a78cdbf, with flag-off assertions and matching translation entries. |
Keep Button as the direct Dropdown child so Tab returns focus after the menu closes, with and without permission hints. Add red-first regression coverage for Amin's focus-return thread and clarify that card hints belong to the Charts and Dashboards lists, not Recently Archived.
aminghadersohi
left a comment
aminghadersohi
left a comment
There was a problem hiding this comment.
The More Options hint is no longer announced to screen readers; details inline.
| aria-label={ariaLabel} | ||
| aria-haspopup="true" | ||
| data-test={dataTest} | ||
| <Tooltip title={tooltipTitle} trigger={['hover', 'focus']}> |
There was a problem hiding this comment.
With the Tooltip wrapping the Dropdown, its aria-describedby no longer reaches the More Options button, so screen readers focusing it don't hear why Edit/Archive is unavailable. Passing a useId as the Tooltip id and as the Button's aria-describedby restores it.
Archive links stay available until dismissed, dependency warnings show accurate totals, and Recently Archived follows browser history consistently. Keyboard users can read why an action is unavailable, and recovery links work under a URL prefix.
#45476 has merged, so this branch now contains only its own changes.
SUMMARY
Follow-ups from a product-acceptance pass over #45476:
Not included: an explicit "this dataset is archived" message in Explore. It needs a server-provided reason, so it'll be a separate change.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Behavior and copy changes. Screenshots to follow.
TESTING INSTRUCTIONS
/analytics), recover a dashboard: the toast link opens it.ADDITIONAL INFORMATION
SOFT_DELETE