From 4878a7fc06b5ca0405f1e946e5a48a8c48e4eb78 Mon Sep 17 00:00:00 2001 From: Pit Friedrich Date: Tue, 28 Jul 2026 23:25:13 +0200 Subject: [PATCH] fix: undo closing a widget (#40) Closing a widget was instant and destructive - one misclick on the X lost its size and position. Close now shows a toast with an undo button that re-adds the very same widget at its captured gs-x/y/w/h, keeping its gs-id. Programmatic close() (fromClient=false) is skipped. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HGRChzremwYCUctf2qMrQz --- .../com/example/components/GridStackItem.java | 35 +++++++++- .../java/com/example/views/DashboardView.java | 60 +++++++++++++++-- .../vaadin-i18n/translations.properties | 2 + .../vaadin-i18n/translations_en.properties | 2 + .../vaadin-i18n/translations_es.properties | 2 + .../e2e/WidgetUndoClosePlaywrightTest.java | 65 +++++++++++++++++++ .../com/example/views/DashboardViewTest.java | 61 +++++++++++++++++ 7 files changed, 219 insertions(+), 8 deletions(-) create mode 100644 src/test/java/com/example/e2e/WidgetUndoClosePlaywrightTest.java diff --git a/src/main/java/com/example/components/GridStackItem.java b/src/main/java/com/example/components/GridStackItem.java index e267351..9935458 100644 --- a/src/main/java/com/example/components/GridStackItem.java +++ b/src/main/java/com/example/components/GridStackItem.java @@ -312,10 +312,26 @@ public class GridStackItem extends Div { } private void close(boolean fromClient) { + // Captured before detaching: once removeFromParent() runs, the grid no + // longer reports this item, so its position could not be read back from + // there afterwards. The gs-* attributes themselves are untouched by the + // detach, so a caller that keeps this item around and re-adds it later + // (e.g. to undo the close) lands it back at exactly this spot. + Position position = capturePosition(); // Detach first so listeners observe the layout they are about to see — // getLayout() on the grid no longer counts this item. getElement().removeFromParent(); - fireEvent(new CloseEvent(this, fromClient)); + fireEvent(new CloseEvent(this, fromClient, position)); + } + + private Position capturePosition() { + return new Position(getItemId(), + attrInt("gs-x"), attrInt("gs-y"), attrInt("gs-w"), attrInt("gs-h")); + } + + private int attrInt(String name) { + String value = getElement().getAttribute(name); + return value == null ? 0 : Integer.parseInt(value); } /** Shows or hides the close button (shown by default). */ @@ -346,10 +362,23 @@ public class GridStackItem extends Div { return addListener(CloseEvent.class, listener); } - /** Fired after the item has been removed from its {@link GridStackLayout}. */ + /** Fired after the item has been removed from its {@link GridStackLayout}. + * Carries the {@link Position} it held right before detaching, so a + * listener can offer to undo the close (re-adding the item at that exact + * spot) without having to track positions itself. {@link #isFromClient()} + * tells apart a user-initiated close (button/menu) from a programmatic + * {@link GridStackItem#close()} — an undo affordance only makes sense for + * the former. */ public static class CloseEvent extends ComponentEvent { - CloseEvent(GridStackItem source, boolean fromClient) { + private final Position position; + + CloseEvent(GridStackItem source, boolean fromClient, Position position) { super(source, fromClient); + this.position = position; + } + + public Position getPosition() { + return position; } } diff --git a/src/main/java/com/example/views/DashboardView.java b/src/main/java/com/example/views/DashboardView.java index ebe6a09..d541590 100644 --- a/src/main/java/com/example/views/DashboardView.java +++ b/src/main/java/com/example/views/DashboardView.java @@ -19,6 +19,7 @@ import com.vaadin.flow.component.button.ButtonVariant; import com.vaadin.flow.component.dialog.Dialog; import com.vaadin.flow.component.html.Paragraph; import com.vaadin.flow.component.html.Span; +import com.vaadin.flow.component.notification.Notification; import com.vaadin.flow.component.orderedlayout.HorizontalLayout; import com.vaadin.flow.component.orderedlayout.VerticalLayout; import com.vaadin.flow.router.HasDynamicTitle; @@ -40,7 +41,8 @@ import java.util.Optional; * layout is persisted per browser, plus controls to add widgets and reset the * layout at runtime. Widgets are removed by their own close button (see * {@link GridStackItem#setClosable(boolean)}), not from the toolbar, and are - * added back from the picker over {@link WidgetRegistry}. + * added back from the picker over {@link WidgetRegistry} — or undone straight + * from the toast every close spawns (see {@link #offerUndo}). *

* All widget numbers come from {@link ChartDataService} and every chart widget * is built by the registry; this view only decides where a widget sits and @@ -97,17 +99,18 @@ public class DashboardView extends VerticalLayout implements HasDynamicTitle { refreshKpiTile(kpi.id()); } }); - grid.add(item); + addWidgetToGrid(item); } context.addFilterChangeListener(this::updateKpiTiles); - grid.add( + List.of( defaultWidget(WidgetRegistry.REVENUE_TREND, 0, 1, 6, 3), defaultWidget(WidgetRegistry.REVENUE_MONTH, 6, 1, 6, 3), defaultWidget(WidgetRegistry.REVENUE_REGION, 0, 4, 5, 3), new GridStackItem("hint", 5, 4, 7, 3, new Card(getTranslation("card.gridstackHint"), - new Paragraph(getTranslation("gridstack.hint"))))); + new Paragraph(getTranslation("gridstack.hint"))))) + .forEach(this::addWidgetToGrid); status.setText(getTranslation("gridstack.statusInitial")); status.addClassName("dialect-muted"); @@ -160,10 +163,57 @@ public class DashboardView extends VerticalLayout implements HasDynamicTitle { // widget must not hand its id to the next one, or the new widget would // inherit the closed one's saved position. extraWidgetCount++; - grid.add(widget(definition.type() + "-" + extraWidgetCount, definition, + addWidgetToGrid(widget(definition.type() + "-" + extraWidgetCount, definition, 0, 0, definition.width(), definition.height())); } + /** Adds an item to the grid and wires the undo toast onto its close — every + * widget goes through here exactly once, so the close listener (and thus + * the undo offer) is registered once per item, not once per re-add: an + * undo hands the very same instance back to {@link #grid} directly. */ + private void addWidgetToGrid(GridStackItem item) { + item.addCloseListener(this::offerUndo); + grid.add(item); + } + + /** + * Closing a widget is one misclick away from losing its position, and a + * confirmation dialog on every close would be worse than the problem — so + * this shows a dismissible undo toast instead. Skipped for programmatic + * closes ({@link GridStackItem#close()}, e.g. from duplicate cleanup), + * which is exactly what {@link GridStackItem.CloseEvent#isFromClient()} + * tells apart. + *

+ * Undo re-adds the very item that was closed — not a rebuilt copy — at the + * {@link GridStackItem.Position} the event captured before detaching, so it + * keeps its {@code gs-id} and its exact size/position rather than landing + * in the grid's next free slot. + */ + private void offerUndo(GridStackItem.CloseEvent event) { + if (!event.isFromClient()) { + return; + } + GridStackItem item = event.getSource(); + GridStackItem.Position position = event.getPosition(); + + Notification toast = new Notification(); + toast.setDuration(8000); + toast.setPosition(Notification.Position.BOTTOM_START); + + Button undo = new Button(getTranslation("gridstack.undo"), e -> { + item.setPosition(position.x(), position.y(), position.w(), position.h()); + grid.add(item); + toast.close(); + }); + undo.addThemeVariants(ButtonVariant.LUMO_TERTIARY_INLINE); + + HorizontalLayout content = new HorizontalLayout( + new Span(getTranslation("gridstack.closed")), undo); + content.setAlignItems(Alignment.CENTER); + toast.add(content); + toast.open(); + } + /** A widget of the initial set. Its id is the plain type — never handed out * again by {@link #addWidget(WidgetDefinition)}, which always suffixes a * counter — and it keeps the dashboard's own placement rather than the diff --git a/src/main/resources/vaadin-i18n/translations.properties b/src/main/resources/vaadin-i18n/translations.properties index 31c698e..21216eb 100644 --- a/src/main/resources/vaadin-i18n/translations.properties +++ b/src/main/resources/vaadin-i18n/translations.properties @@ -37,6 +37,8 @@ gridstack.restore=Wiederherstellen gridstack.duplicate=Duplizieren gridstack.remove=Entfernen gridstack.export=Als CSV exportieren +gridstack.closed=Widget entfernt +gridstack.undo=Rückgängig # Spaltenüberschrift der CSV-Exporte; die Wertspalte trägt den Serien-Namen. export.category=Kategorie diff --git a/src/main/resources/vaadin-i18n/translations_en.properties b/src/main/resources/vaadin-i18n/translations_en.properties index 0ccc49c..ac6f1f6 100644 --- a/src/main/resources/vaadin-i18n/translations_en.properties +++ b/src/main/resources/vaadin-i18n/translations_en.properties @@ -37,6 +37,8 @@ gridstack.restore=Restore gridstack.duplicate=Duplicate gridstack.remove=Remove gridstack.export=Export as CSV +gridstack.closed=Widget removed +gridstack.undo=Undo # Column header of the CSV exports; the value column carries the series name. export.category=Category diff --git a/src/main/resources/vaadin-i18n/translations_es.properties b/src/main/resources/vaadin-i18n/translations_es.properties index ba3e5df..181bbf2 100644 --- a/src/main/resources/vaadin-i18n/translations_es.properties +++ b/src/main/resources/vaadin-i18n/translations_es.properties @@ -37,6 +37,8 @@ gridstack.restore=Restaurar gridstack.duplicate=Duplicar gridstack.remove=Eliminar gridstack.export=Exportar como CSV +gridstack.closed=Widget eliminado +gridstack.undo=Deshacer # Encabezado de columna de las exportaciones CSV; la columna de valores lleva # el nombre de la serie. diff --git a/src/test/java/com/example/e2e/WidgetUndoClosePlaywrightTest.java b/src/test/java/com/example/e2e/WidgetUndoClosePlaywrightTest.java new file mode 100644 index 0000000..29c9ab9 --- /dev/null +++ b/src/test/java/com/example/e2e/WidgetUndoClosePlaywrightTest.java @@ -0,0 +1,65 @@ +package com.example.e2e; + +import com.microsoft.playwright.Locator; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static com.microsoft.playwright.assertions.PlaywrightAssertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * End-to-end coverage for the undo-on-close toast (see + * {@code DashboardView#offerUndo}): a real browser is what actually confirms + * the item lands back in its exact grid cell, gridstack animation included. + */ +class WidgetUndoClosePlaywrightTest extends PlaywrightTestBase { + + @BeforeEach + void openDashboard() { + navigate(""); + } + + @Test + void close_thenUndo_restoresTheExactSlot() { + Locator item = widget(); + String before = slot(item); + + closeButton(item).click(); + assertThat(widget()).not().isAttached(); + + undoButton().click(); + + Locator restored = widget(); + assertThat(restored).isAttached(); + page.waitForCondition(() -> before.equals(slot(restored))); + assertEquals(before, slot(restored), "undo must restore the exact position and size"); + } + + /** Targeted by its stable {@code gs-id} rather than by position: closing it + * changes which item a positional selector would resolve to first. */ + private Locator widget() { + return page.locator(".grid-stack-item[gs-id='revenue-trend']"); + } + + private Locator closeButton(Locator item) { + return item.locator("> .dialect-close-button"); + } + + private Locator undoButton() { + return page.locator("vaadin-notification-card").getByRole( + com.microsoft.playwright.options.AriaRole.BUTTON, + new Locator.GetByRoleOptions().setName("Undo")); + } + + /** The item's box measured against the grid, not the viewport — same as + * {@code WidgetActionMenuPlaywrightTest#slot}. */ + private String slot(Locator item) { + return (String) item.evaluate(""" + el => { + const grid = el.closest('.grid-stack').getBoundingClientRect(); + const box = el.getBoundingClientRect(); + return [box.left - grid.left, box.top - grid.top, box.width, box.height] + .map(Math.round).join(','); + }"""); + } +} diff --git a/src/test/java/com/example/views/DashboardViewTest.java b/src/test/java/com/example/views/DashboardViewTest.java index dc52cd2..6643b51 100644 --- a/src/test/java/com/example/views/DashboardViewTest.java +++ b/src/test/java/com/example/views/DashboardViewTest.java @@ -25,6 +25,7 @@ import com.vaadin.flow.component.contextmenu.MenuItem; import com.vaadin.flow.component.dialog.Dialog; import com.vaadin.flow.component.html.Anchor; import com.vaadin.flow.component.html.Div; +import com.vaadin.flow.component.notification.Notification; import com.vaadin.flow.dom.DomEvent; import com.vaadin.flow.internal.JacksonUtils; import org.junit.jupiter.api.Test; @@ -267,6 +268,66 @@ class DashboardViewTest extends SpringBrowserlessTest { assertEquals(DEFAULT_WIDGETS - 1, grid.getLayout().size()); } + @Test + void closingAWidget_showsAnUndoToast() { + navigate(DashboardView.class); + + clickCloseButton(itemById("revenue-trend")); + + Notification toast = $(Notification.class).first(); + assertNotNull(toast, "closing a widget must offer a way back"); + assertNotNull($(Button.class).from(toast).withText(translate("gridstack.undo")).first()); + } + + @Test + void undo_restoresTheWidgetAtItsOriginalPositionAndId() { + navigate(DashboardView.class); + GridStackLayout grid = $view(GridStackLayout.class).first(); + GridStackItem.Position before = positionOf("revenue-trend"); + + clickCloseButton(itemById("revenue-trend")); + assertEquals(DEFAULT_WIDGETS - 1, grid.getLayout().size()); + + clickUndo(); + + assertEquals(DEFAULT_WIDGETS, grid.getLayout().size()); + assertEquals(before, positionOf("revenue-trend"), + "undo must restore the exact position and size, not the next free slot"); + assertEquals(1, $view(LineChart.class).all().size(), "the chart itself is back too"); + } + + /** Closing the same widget twice in a row — once, undoing, then again — + * must still offer an undo the second time: the close listener is + * registered once per item, not once per (re-)add. */ + @Test + void undoneWidget_offersAnUndoAgainWhenClosedAnotherTime() { + navigate(DashboardView.class); + + clickCloseButton(itemById("revenue-trend")); + clickUndo(); + clickCloseButton(itemById("revenue-trend")); + + assertNotNull($(Button.class).from($(Notification.class).first()) + .withText(translate("gridstack.undo")).first()); + } + + /** A close triggered from code (not the button or the menu) is not a + * misclick, so it must not spawn a toast the user never asked for. */ + @Test + void programmaticClose_doesNotShowAnUndoToast() { + navigate(DashboardView.class); + + itemById("revenue-trend").close(); + + assertTrue($(Notification.class).all().isEmpty(), + "a programmatic close must not offer an undo"); + } + + private void clickUndo() { + $(Button.class).from($(Notification.class).first()) + .withText(translate("gridstack.undo")).first().click(); + } + @Test void nonClosableItem_hasNoCloseButton() { navigate(DashboardView.class); -- 2.52.0