sadpandajoe commented on code in PR #45090: URL: https://github.com/apache/superset/pull/45090#discussion_r4259500628
########## superset-frontend/src/features/canvas/PlanModeButton.tsx: ########## @@ -0,0 +1,80 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { getClientErrorObject, SupersetClient } from '@superset-ui/core'; +import { Button } from '@superset-ui/core/components'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; + +/** + * What entering a draft is called in the UI. A working name, still being + * settled; changing it here changes it everywhere it is shown. + */ +export const PLAN_MODE_LABEL = () => t('Plan mode'); + +interface PlanModeButtonProps { + canvasId: number; + /** Called with the new draft's token, to open it on this page. */ + onStarted: (token: string) => void; +} + +/** + * Starts a plan: a private draft of the canvas, which is where changes are + * made. The published canvas itself is read-only, so this is how a user with + * edit permission gets somewhere they can drag, resize and ask for changes. + * The draft is published or discarded from the banner it shows. + */ +export default function PlanModeButton({ + canvasId, + onStarted, +}: PlanModeButtonProps) { + const { addDangerToast } = useToasts(); + const [starting, setStarting] = useState(false); + + const start = async () => { + setStarting(true); + try { + const { json } = await SupersetClient.post({ + endpoint: `/api/v1/canvas/${canvasId}/draft`, + }); + const token = json?.result?.token; + if (typeof token === 'string') { + onStarted(token); Review Comment: `onStarted` fires whenever the POST resolves, with no check that the user is still on the canvas that started it. Click Plan mode on `/canvas/7`, navigate to `/canvas/8` before the response, and `draft.open` writes canvas 7's token into canvas 8's URL. `useCanvasDefinition` fetches the draft by token alone, so canvas 8's page renders canvas 7's draft (and Publish targets canvas 7). Could the callback be ignored once `canvasId` has changed or the component has unmounted? ########## superset-frontend/src/features/canvas/CanvasBody.test.tsx: ########## @@ -0,0 +1,238 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * The wiring that keeps a layout edit from being lost by a publish, exercised + * end to end rather than prop by prop: the layout hook's `saving` reaching the + * banner's Publish button, and the banner's busy state reaching `editable`. + * Asserting those props in isolation let either side be deleted silently. + */ +import fetchMock from 'fetch-mock'; +import { canvas } from 'src/core'; +import { + fireEvent, + render, + screen, + waitFor, +} from 'spec/helpers/testing-library'; +import CanvasBody from './CanvasBody'; +import { emptyScopeValues } from './CanvasGrid'; +import type { CanvasDefinitionResult, GridPlacement } from './types'; + +const draftEndpoint = 'glob:*/api/v1/canvas/draft'; +const commitEndpoint = 'glob:*/api/v1/canvas/draft/commit'; + +const KPI: GridPlacement = { col: 1, row: 1, colSpan: 6, rowSpan: 4 }; +const TREND: GridPlacement = { col: 1, row: 5, colSpan: 24, rowSpan: 8 }; +const WIDTH = 1000; +const COL_PITCH = 632 / 24 + 16; + +const result = { + version: 1, + revision: 0, + definition: { + version: 1, + root: { + layout: { columns: 24, gap: 16, rowUnit: 40 }, + children: ['kpi', 'trend'], + }, + nodes: { + kpi: { + widgetType: 'test.plain', + schemaVersion: 1, + props: {}, + layout: {}, + }, + trend: { + widgetType: 'test.plain', + schemaVersion: 1, + props: {}, + layout: {}, + }, + }, + interactions: { filters: {}, crossFilters: {}, customizations: {} }, + settings: { + refresh: { interval: 0, stagger: 0, exempt: [] }, + colors: { labelColors: {} }, + display: { showTimestamps: false }, + crossFilters: { enabled: true }, + }, + }, + filterScopes: {}, + crossFilterScopes: {}, + customizationScopes: {}, + placements: { kpi: KPI, trend: TREND }, + widgetTypes: { kpi: 'test.plain', trend: 'test.plain' }, + gridColumns: {}, + layoutConstraints: {}, + canEdit: true, +} as unknown as CanvasDefinitionResult; + +const registration = canvas.registerWidgetRenderer('test.plain', () => ( + <div>widget</div> +)); +afterAll(() => registration.dispose()); + +beforeAll(() => { + Object.defineProperty(HTMLElement.prototype, 'clientWidth', { + configurable: true, + get: () => WIDTH, + }); + HTMLElement.prototype.setPointerCapture = jest.fn(); + HTMLElement.prototype.releasePointerCapture = jest.fn(); +}); + +beforeEach(() => { + fetchMock.removeRoutes(); + fetchMock.clearHistory(); +}); + +class TestPointerEvent extends MouseEvent { + pointerId: number; + + constructor( + type: string, + init: MouseEventInit & { pointerId?: number } = {}, + ) { + super(type, { bubbles: true, cancelable: true, ...init }); + this.pointerId = init.pointerId ?? 1; + } +} + +/** A whole drag on the first move handle, from grab to release. */ +const dragFirstWidget = (dx: number) => { + const handle = screen.getAllByRole('button', { name: 'Move widget' })[0]; + [ + new TestPointerEvent('pointerdown', { button: 0, clientX: 0, clientY: 0 }), + new TestPointerEvent('pointermove', { clientX: dx, clientY: 0 }), + new TestPointerEvent('pointerup', { clientX: dx, clientY: 0 }), + ].forEach(event => fireEvent(handle, event)); +}; + +const renderBody = () => + render( + <CanvasBody + canvasId={7} + result={result} + reload={jest.fn()} + values={emptyScopeValues()} + onValueChange={jest.fn()} + refreshKeys={{}} + draftToken="tok-123" Review Comment: Every test in this file renders with `draftToken="tok-123"`, so the `Boolean(draftToken)` half of `editable` in `CanvasBody.tsx` is never exercised. Dropping it would give a published canvas drag/resize handles for any `canEdit` user and these tests would still pass. Could a case render `canEdit: true` with no token and assert neither the move nor the resize handle is present? ########## superset-frontend/src/features/canvas/useDraftToken.test.tsx: ########## @@ -0,0 +1,68 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { act } from 'react'; +import { BrowserRouter } from 'react-router-dom'; +import { renderHook } from '@testing-library/react'; +import { useDraftToken } from './useDraftToken'; + +/** + * `BrowserRouter`, not `MemoryRouter`: the hook reads the token from + * `window.location.hash`, which an in-memory history never touches. The app + * runs on a browser router, where the router and the address bar agree. + */ +const atHash = (hash: string) => { + window.location.hash = hash; + return renderHook(() => useDraftToken(), { + wrapper: ({ children }) => <BrowserRouter>{children}</BrowserRouter>, + }); +}; + +afterEach(() => { + window.location.hash = ''; +}); + +test('the token is read from the url fragment', () => { + const { result } = atHash('#draft=tok-123'); + + expect(result.current.token).toBe('tok-123'); +}); + +test('no fragment means no draft is open', () => { + expect(atHash('').result.current.token).toBeUndefined(); + // A fragment that isn't a draft token is ignored. + expect(atHash('#other=1').result.current.token).toBeUndefined(); +}); + +test('opening a draft exposes its token straight away', () => { + // Replacing the hash fires no `hashchange`, so the hook cannot rely on the + // listener alone -- the token has to be set when `open` is called. + const { result } = atHash(''); + + act(() => result.current.open('tok-456')); + + expect(result.current.token).toBe('tok-456'); Review Comment: This only asserts the hook's local state after `open`. Removing the `history.replace` call in `open` (the part that makes a plan shareable and survive a refresh) leaves this test green. Could it also assert `window.location.hash === '#draft=tok-456'`? ########## superset-frontend/src/features/canvas/CanvasGridSurface.tsx: ########## @@ -0,0 +1,486 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * @fileoverview One grid of a canvas, with drag and resize built in. + * + * There is no edit mode: a user who may edit gets a drag grip and a resize + * corner on each widget, on the canvas as they view it. A user who may not + * gets the same grid with no handles. Dragging never covers the widget's own + * surface, so a filter or chart underneath stays clickable. + * + * While a gesture is live the widget follows the pointer and a dashed outline + * marks the cells it would take; releasing persists exactly those cells. + */ + +import { + PointerEvent as ReactPointerEvent, + KeyboardEvent as ReactKeyboardEvent, + ReactNode, + useCallback, + useEffect, + useRef, + useState, +} from 'react'; +import { css, styled } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import { Icons } from '@superset-ui/core/components'; +import { + GridMetrics, + gridDelta, + movedPlacement, + NO_DELTA, + overlappedSiblings, + resizedPlacement, + samePlacement, +} from './gridGeometry'; +import type { GridPlacement, SpanConstraints } from './types'; + +type GestureKind = 'move' | 'resize'; + +interface Gesture { + nodeId: string; + kind: GestureKind; + pointerId: number; + startX: number; + startY: number; + dx: number; + dy: number; + /** + * Where the widget sat when it was grabbed. The gesture resolves against + * this, not against the current placement: a write landing mid-gesture (a + * sibling's drop pushing this one down, or a draft poll) would otherwise + * move the ground under the pointer and persist the drop relative to a row + * the user never aimed at. + */ + origin: GridPlacement; +} + +const Grid = styled.div<GridMetrics>` + display: grid; + grid-template-columns: repeat(${({ columns }) => columns}, minmax(0, 1fr)); + grid-auto-rows: ${({ rowUnit }) => rowUnit}px; + gap: ${({ gap }) => gap}px; +`; + +const GridItem = styled.div<{ + placement?: GridPlacement; + dragging?: boolean; + offset?: { x: number; y: number }; +}>` + ${({ placement, dragging, offset }) => css` + min-width: 0; + min-height: 0; + position: relative; + ${ + placement && + css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + ` + } + ${ + dragging && + offset && + css` + /* Follow the pointer without reflowing the grid under it. */ + transform: translate(${offset.x}px, ${offset.y}px); + z-index: 2; + opacity: 0.85; + ` + } + `} +`; + +/** + * Where the widget being dragged would land. Turns red once it covers a + * sibling, so the user sees an overlap before releasing rather than being + * surprised when the server pushes a widget down to resolve it. + */ +const DropPreview = styled.div<{ + placement: GridPlacement; + colliding?: boolean; +}>` + ${({ theme, placement, colliding }) => css` + grid-column: ${placement.col} / span ${placement.colSpan}; + grid-row: ${placement.row} / span ${placement.rowSpan}; + border: 2px dashed ${colliding ? theme.colorError : theme.colorPrimary}; + border-radius: ${theme.borderRadius}px; + background: ${colliding ? theme.colorErrorBg : theme.colorPrimaryBg}; + pointer-events: none; + z-index: 1; + `} +`; + +/** + * Wraps a widget so its handles can sit on top of it. The handles stay hidden + * until the pointer is over the widget or a handle has focus, so a canvas at + * rest looks the same whether or not the user may edit it. + */ +const Interactive = styled.div` + ${({ theme }) => css` + height: 100%; + position: relative; + + /* Direct children only: hovering a container must not reveal the + handles of every widget nested inside it. */ + & > .canvas-handle { + opacity: 0; + transition: opacity ${theme.motionDurationMid}; + } + + &:hover > .canvas-handle, + & > .canvas-handle:focus-visible { + opacity: 1; + } + `} +`; + +const Grip = styled.button` + ${({ theme }) => css` + position: absolute; + top: ${theme.sizeUnit}px; Review Comment: The drag grip sits at `top/right: sizeUnit` with `z-index: 3`, over the existing More Options button in `WithQuery` (`top: 0; right: 0; z-index: 1`). In an editable draft, a widget with a query shows both in the same corner, and clicking the ellipsis lands on the grip and starts a drag instead of opening View query. Is there room to offset the grip (or the menu) so both stay clickable? ########## superset-frontend/src/features/canvas/DraftBanner.tsx: ########## @@ -85,8 +102,11 @@ export default function DraftBanner({ <Button buttonStyle="primary" buttonSize="small" - loading={busy} + loading={busy || saving} + disabled={saving} Review Comment: Publish is held back while a layout save is in flight, but Discard stays enabled. If someone drags a widget and clicks Discard before the PATCH settles, the DELETE can land between the PATCH's `load_draft` and its `_save()` (`superset/commands/canvas/draft.py`). The save then recreates the cache entry, so "Draft discarded" is shown while the old token still opens the supposedly discarded changes. Should Discard also wait on `saving`, the way Publish does? -- 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]
