michael-s-molina commented on code in PR #42865:
URL: https://github.com/apache/superset/pull/42865#discussion_r3735849074
##########
superset-frontend/packages/superset-core/src/dashboard/index.ts:
##########
@@ -53,21 +53,52 @@
import type { Event } from '../common';
+/**
+ * How a container arranges its own children.
+ *
+ * - `grid` (the default, and what every node written before this field
+ * existed still means) — children occupy cells of the container's column
+ * grid and are compacted upward, so the space a removed or moved block
+ * leaves behind closes itself.
+ * - `free` — the same cells, uncompacted. A child stays exactly where it was
+ * put and may overlap a sibling. It reads the same four child fields
+ * `grid` does, so switching a container between the two never discards a
+ * position an author or an agent set.
+ * - `flex` — children flow along a line and wrap, sharing that line in
+ * proportion to their `colSpan` instead of occupying named cells. Their
+ * position is their order in `children`, not a coordinate.
+ *
+ * A flow is still not a mode, and the two that are here are the two that
+ * genuinely could not be expressed as a grid: `free` because compaction is a
+ * property of the container rather than of any child's coordinates, and
+ * `flex` because a proportional line has no cells to name.
+ */
+export type LayoutMode = 'grid' | 'free' | 'flex';
Review Comment:
Can you remove Canvas building block and layout modes? Containers are
currently being discussed as part of the layout efforts.
##########
superset-frontend/packages/superset-core/src/chat/index.ts:
##########
@@ -151,6 +151,57 @@ export declare const onDidChangeDisplayMode:
Event<DisplayMode>;
*/
export declare const onDidResizePanel: Event<{ width: number }>;
-// TODO: client actions API — tool availability functions will be added here
-// once the client_actions SIP is finalized. The chat namespace is the
-// intended integration point between the two SIPs.
+/** The normalized answer returned to the assistant by a browser-owned tool. */
+export interface ClientToolResult {
+ content: string;
+ isError?: boolean;
+}
+
+/** The part of a browser-owned tool definition that is safe to send to a
model. */
+export interface ClientToolSpec {
+ /** Unique tool name. Later registrations take precedence on collisions. */
+ name: string;
+ /** Explain when the model should use the tool and what it returns. */
+ description: string;
+ /** JSON Schema describing the tool's argument object. */
+ inputSchema: Record<string, unknown>;
+}
+
+/**
+ * A tool implemented by the page in the user's browser.
+ *
+ * The handler is never sent to the chat extension's backend. It executes in
+ * the active Superset page and should return concise, model-readable content.
+ */
+export interface ClientTool extends ClientToolSpec {
Review Comment:
Can you remove client tools registration? These are kept inside the
extension for now. @justinpark is reviewing the SIP and will add the APIs later
and at the same time remove the extension code related to that part. Same for
the changes in ChatProvider.
##########
superset-frontend/src/core/dashboard/blocks/FlexCanvas.tsx:
##########
@@ -0,0 +1,183 @@
+/**
+ * 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 { useCallback, useState } from 'react';
Review Comment:
Let's remove this one as well.
##########
superset-frontend/src/core/dashboard/blocks/CanvasBlock.tsx:
##########
@@ -237,9 +295,24 @@ export default function CanvasBlock({ nodeId }: { nodeId:
string }) {
// a few mouse-move events. `"vertical"` resolves the same collision
// by moving the sibling down exactly once, by exactly its own
// height, every time.
- compactType="vertical"
+ //
Review Comment:
Let's remove this block for now.
##########
superset-frontend/src/pages/DashboardBuilderV2/CanvasControls.tsx:
##########
@@ -0,0 +1,102 @@
+/**
+ * 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 type { ReactElement } from 'react';
+import { t } from '@apache-superset/core/translation';
+import { useTheme } from '@apache-superset/core/theme';
+import { Button } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import { provider } from 'src/core/dashboard/store';
+import Inert from './InertControl';
+
+/**
+ * What acts on the canvas as a whole, in the canvas's own corner.
+ *
+ * These two are not chrome about the dashboard — not what it is called, who
+ * owns it, or whether it is published. They act on the blocks in front of you,
+ * and both are reached for while looking at them, which is why they sit here
+ * rather than on the bar above.
+ *
+ * **Arrange** is a route, not a control. How a container lays out its
+ * children is a property of that container and is asked with the rest of them
+ * — the columns, the gap, the row height it works alongside. That is the
+ * right home for it and also further from hand than something permanently on
+ * screen, so this is the way back to it. A second copy of the switcher would
+ * be a second thing to keep agreeing with the first; selecting the root is
+ * all this does, and the editor panel brings Properties forward on a
+ * selection it did not make itself. It stays live on a page where almost
+ * nothing is, because selecting a node this page already holds in memory
+ * needs no dashboard row.
+ *
+ * **Refresh** is the opposite: named, and honest that it cannot work. There
+ * is no row behind this page and no query to re-run, so it says so rather
+ * than doing nothing quietly.
+ */
+export default function CanvasControls(): ReactElement {
Review Comment:
We can remove this as well.
##########
superset-frontend/src/core/dashboard/DashboardProvider.ts:
##########
@@ -268,24 +337,68 @@ class DashboardProvider {
// drag-based reparenting (see `CanvasBlock`'s `handleDragStop`) already
// resets exactly these two things on drop; this is that same reset,
// applied here so the programmatic path gives the same guarantee.
- const node = nodes[id];
- const destColumns = targetParent.layout?.columns ?? DEFAULT_COLUMNS;
- nodes[id] = {
- ...node,
- layout: {
- ...node.layout,
- col: undefined,
- row: undefined,
- colSpan:
- node.layout?.colSpan != null
- ? Math.min(node.layout.colSpan, destColumns)
- : undefined,
- },
- };
+ //
+ // None of which is true when the parent has not changed. A move within
+ // one container is a reorder — how a free canvas says "put this in
+ // front", since paint order there is child order — and the position it
+ // keeps is the one the author placed it at. Resetting it would teleport
+ // the block to auto-placement as the price of raising it.
+ if (oldParentId !== newParentId) {
+ const node = nodes[id];
+ const destColumns = targetParent.layout?.columns ?? DEFAULT_COLUMNS;
+ nodes[id] = {
+ ...node,
+ layout: {
+ ...node.layout,
+ col: undefined,
+ row: undefined,
+ colSpan:
+ node.layout?.colSpan != null
+ ? Math.min(node.layout.colSpan, destColumns)
+ : undefined,
+ },
+ };
+ }
this.commit(nodes);
}
+ /**
+ * Which of its siblings a node is drawn over.
+ *
+ * Where children overlap — a `free` canvas — the container's child order is
+ * the paint order, because `react-grid-layout` gives an overlapping item no
+ * `z-index` of its own and the browser falls back to tree order. So "in
+ * front" is "last", and raising a block is reordering it.
+ *
+ * Named for what an author means rather than left to callers to express as
+ * an index, because the index is a trap: {@link moveBuildingBlock} detaches
+ * before it inserts, so the array a node lands in is one shorter than the
+ * one that was counted. Every caller getting that arithmetic right
+ * separately is a bug waiting for the second caller.
+ *
+ * Doing nothing when there is nothing to do is part of the contract, not an
+ * optimisation. A drag ends on the block already in front more often than
+ * not, and a revision tick there re-renders every subscriber to produce the
+ * array it already had.
+ */
+ private restack(id: string, edge: 'front' | 'back'): void {
Review Comment:
Let's not introduce z-indexes for now and keep things simple by supporting
grid layout only for now.
--
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]
