fix(dashboard): disable Discard button when there are no unsaved changes#40832
Conversation
dosubot
Bot
added
change:frontend
Code Review Agent Run #e532dfActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40832 +/- ##
==========================================
- Coverage 65.19% 65.18% -0.01%
==========================================
Files 2767 2767
Lines 156062 156062
Branches 35717 35717
==========================================
- Hits 101741 101735 -6
- Misses 52361 52367 +6
Partials 1960 1960
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.
Pull request overview
This PR updates the dashboard edit toolbar so the Discard action is disabled when the dashboard has no unsaved changes, aligning it with the existing Save button behavior and preventing “discard” from appearing actionable on a pristine edit session.
Changes:
- Disable the Discard button when
hasUnsavedChangesis false. - Update the Header unit test to assert the Discard button is disabled in that state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| superset-frontend/src/dashboard/components/Header/index.tsx | Disables the Discard button based on hasUnsavedChanges. |
| superset-frontend/src/dashboard/components/Header/Header.test.tsx | Updates the related test expectation for the Discard button disabled state. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #472d7dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| <Button | ||
| css={discardBtnStyle} | ||
| buttonSize="small" | ||
| disabled={!hasUnsavedChanges} |
|
Thanks for tackling this, @massucattoj. As noted, sadpandajoe's question above still stands, with both Save and Discard disabled there's no way out of edit mode until you actually change something. I don't think we can merge until that's settled with @yousoph and @kasiazjc. Might be as simple as keeping Discard enabled (just relabeled) when there's nothing to discard, or maybe a separate "exit edit mode" action. Thoughts? |
|
@rusackas Yeah I think this needs a design call before we change anything. I'd rather wait for @yousoph and @kasiazjc to weigh in on @sadpandajoe question that guess at a solution here. I'll ping them on Slack in the next few days to unblock this. |
There was a problem hiding this comment.
I think we should just plow ahead. It's a small, clean fix, and the two new tests lock in both button states. LG(enough)TM, approving, will merge once CI's green.
To close the loop on sadpandajoe's question above: I traced the Header, and it's true that with nothing changed there's no toolbar exit from edit mode, and the "..." menu doesn't have one either. But with nothing unsaved there's no navigation guard, so a refresh or any nav click gets you out. That seems like a smaller wart than a Discard button that discards nothing, so let's take this as-is. We'll put up a follow-up PR giving the pristine state an exit affordance, and @yousoph / @kasiazjc can steer the design from there.
|
Bito Automatic Review Skipped – PR Already Merged |
|
Opened a follow-up on #42208 if anyone wants to take a looksee at that one :D |
SUMMARY
The Discard button in the dashboard edit toolbar was always enabled, even on a freshly opened new dashboard with no changes. Clicking it suggested "discard this dashboard", but the handler only strips
?editfrom the URL the dashboard itself stays persisted (the backend creates it eagerly at/dashboard/new/).Scope note: the ticket description also mentions "Discarding should abort creation and not persist a dashboard." That is not addressed here it would require changing how
/dashboard/new/works (itcurrently eagerly inserts a
DashboardModelbefore any user interaction). Maybe worth a separate ticket?BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
?edit, returns to view mode.ADDITIONAL INFORMATION