Add a new query panel #2393

bharatkashyap · 2023-07-30T18:15:01Z

Closes: #2138

Screen.Recording.2023-10-24.at.8.33.57.PM.mov

I've read and followed the contributing guide on how to create great pull requests.
I've updated the relevant documentation for any new or updated feature
I've linked relevant GitHub issue with "Closes #"
I've added a visual demonstration under the form of a screenshot or video

TODO

This reverts commit 964cc01.

This reverts commit 34ad45c.

…er-left

bharatkashyap · 2023-12-13T17:57:54Z

Ok managed to get it to work. Cleaned out node modules and pulled the latest changes.

Some things I noticed:

Screen.Recording.2023-12-12.at.10.31.22.mov

Resizing doesn't work

When opening page parameters, the panel closes, even with unsaved changes

Fixed both of those; the resizing was a strange bug I didn't completely investigate but got around for now. The page parameters dialog I think should be allowed to coexist with an open panel, so I made that behaviour happen.

Janpot · 2023-12-14T08:45:11Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/ComponentCatalog/ComponentCatalog.tsx

@@ -40,11 +40,11 @@ const FUTURE_COMPONENTS = new Map<string, FutureComponentSpec>([
  ['Radio', { url: 'https://github.com/mui/mui-toolpad/issues/744', displayName: 'Radio' }],
 ]);

-const WIDTH_COLLAPSED = 40;
+export const COMPONENT_CATALOG_WIDTH_COLLAPSED = 40;


No need to export.

Suggested change

export const COMPONENT_CATALOG_WIDTH_COLLAPSED = 40;

const COMPONENT_CATALOG_WIDTH_COLLAPSED = 40;

…olpad into query-explorer-left

Janpot · 2023-12-14T09:01:46Z

packages/toolpad-app/src/components/EditableText.tsx

@@ -73,15 +75,21 @@ const EditableText = React.forwardRef<HTMLInputElement, EditableTextProps>(

    const handleChange = React.useCallback(
      (event: React.ChangeEvent<HTMLInputElement>) => {
+        if (readOnly) {


If this gets called while the input is readonly, that would mean there is an error in the code, right?
In that case, would it make more sense to make this an invariant?

Suggested change

if (readOnly) {

invariant(!readOnly, 'Readonly input should be disabled')

(Same below)

Janpot

Went through all the files. This looks great! I sprinkled a few comments here and there, nothing blocking.

Janpot · 2023-12-14T09:08:49Z

packages/toolpad-app/src/runtime/ToolpadApp.tsx

@@ -563,8 +563,12 @@ function parseBindings(
    }

    if (appDom.isQuery(elm)) {
+      let kind: 'query' | 'action' = 'query';


Remark Is there value in introducing another name for "mutation"? We could also keep calling it "mutation" in the code and just use "Action" as a display name? It could keep the code simpler.

I'm marking this with Remark. It's just a stylistic comment on the code. These are just things that go through my head when reading the code. Doesn't require a change.

Understood, I added "action" as a way to progressively perhaps do away with mutation in the code and eventually create a separate node type called "action", since if that is what we want to call it in the UI, should it also not be what we describe it in the codebase for simplicity?

Personally I prefer either one way or the other:

either we call it "action" everywhere and just do a translation to "mutation", right before we read/write to a file

or we call it "mutation" everywhere and name it "action" just right before we display in the UI

The reason is that this prevents us from mismatching and constant translation all around the codebase. Ideally if the rename is done in a large PR, you start with the latter, just to avoid introducing many unnecessary chamges to unrelated code. Then in a separate PR update the codebase to the former.

But, leave it for now, I marked it as a "note" it's not blocking the merge of this PR.

Janpot · 2023-12-14T09:15:50Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueriesExplorer/index.tsx

+    onSelectNode?.(toolpadNodeId);
+  }, [onSelectNode, toolpadNodeId]);
+
+  const queryCreationMode = React.useMemo(() => {


Note Ideally, the DataTreeItem doesn't know if it's a query or a mutation. In a true abstraction, we should be able to introduce a third query type without needing to alter DataTreeItem.

Also, I don't fully understand, do all tree nodes in the queries tree have the same node id? ids must be unique, otherwise we can't refer to the individual items in the treeview APIs. And in an ideal world they are opaque as well. (Meaning: we can't gain any knowledge about the object just by inspecting the id on its own)

Ideally, the DataTreeItem doesn't know if it's a query or a mutation. In a true abstraction, we should be able to introduce a third query type without needing to alter DataTreeItem.

Agreed, naming it DataTreeItem was an attempt to begin making it agnostic to the type of item represented but I imagine that'll have to be a separate effort distinct from this one or any future improvements scoped to the query panel

Also, I don't fully understand, do all tree nodes in the queries tree have the same node id? ids must be unique, otherwise we can't refer to the individual items in the treeview APIs. And in an ideal world they are opaque as well. (Meaning: we can't gain any knowledge about the object just by inspecting the id on its own)

The node ids are separate for each node

Janpot · 2023-12-14T09:21:15Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueryEditor/QueryEditorPanel.tsx

+import { BindableAttrValue, LiveBinding } from '@mui/toolpad-core';
+import { useBrowserJsRuntime } from '@mui/toolpad-core/jsBrowserRuntime';
+import invariant from 'invariant';
+


Suggested change

Janpot · 2023-12-14T09:22:51Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueryEditor/QueryEditorPanel.tsx

+      }}
+    >
+      <Stack
+        display={'grid'}


Looking for a lint rule that can catch these

Suggested change

display={'grid'}

display="grid"

Janpot · 2023-12-14T09:23:58Z

packages/toolpad-app/src/toolpad/AppEditor/PageEditor/QueryEditor/QueryEditorPanel.tsx

+          value={draft?.attributes?.mode ?? 'query'}
+          onChange={handleModeChange}
+          sx={{
+            '& .MuiInputLabel-root': { fontSize: 12 },


Suggested change

'& .MuiInputLabel-root': { fontSize: 12 },

[`& .${inputLabelClasses.root}`]: { fontSize: 12 },

?

I noticed this all around the PR, but I only mentioned it once.
Non-blocking as well

Janpot · 2023-12-14T09:48:22Z

packages/toolpad-app/src/toolpadDataSources/googleSheets/client.tsx

@@ -81,7 +81,7 @@ function QueryEditor({

  const handleSpreadsheetChange = React.useCallback(
    (event: React.SyntheticEvent<Element, Event>, newValue: GoogleDriveFile | null) => {
-      setInput((existing) => {
+      setInput?.((existing) => {


Why did this change? What is the purpose of a query editor without an onChange prop defined?

The onChange functionality for the new query editor client started living in the appStateApi, so we don't need to pass in an onChange handler prop anymore

But functionally, how should it behave when no handler is defined. What's the use-case?

Janpot · 2023-12-14T09:50:36Z

packages/toolpad-app/src/toolpadDataSources/local/client.tsx

The function selection control belongs in its own file.

Janpot · 2023-12-14T09:51:43Z

packages/toolpad-app/src/toolpadDataSources/local/types.ts

@@ -1,8 +1,9 @@
 import { BindableAttrValue, ExecFetchResult, PrimitiveValueType } from '@mui/toolpad-core';
 import type { IntrospectionResult } from '../../server/functionsTypesWorker';

-export interface LocalConnectionParams {}
+export type FileTypes = 'query' | 'component' | 'page' | 'theme';


Is this being used?

Suggested change

export type FileTypes = 'query' | 'component' | 'page' | 'theme';

Janpot · 2023-12-14T09:54:40Z

packages/toolpad-app/src/toolpadDataSources/mysql/client.tsx

@@ -12,6 +12,7 @@ function MySQLConnectionParamsInput({

 const dataSource: ClientDataSource<SqlConnectionParams, SqlQuery> = {
  displayName: 'MySQL',
+  isEnabled: false,


Instead of this property, why not just omit them where we're exporting all the datasources?

in any case, everywhere in the codebase "enabled"/"disabled" seem one of this names where the "is" prefix is always implied.

Agreed, this can be done without

Janpot · 2023-12-14T09:56:49Z

packages/toolpad-app/src/constants.ts

@@ -18,6 +18,8 @@ export const TOOLPAD_BRIDGE_GLOBAL = '__TOOLPAD_BRIDGE__';

 export const VERSION_CHECK_INTERVAL = 1000 * 60 * 10;

+export const PAGE_PANEL_WIDTH = 250;


Suggested change

export const PAGE_PANEL_WIDTH = 250;

Do you mean directly defining the width in the consuming file instead of creating a constant for this?

Yes, it's still a constant, but no need for it globally. It's used in only one file. And it's the sort of thing that probably should always be local.

…olpad into query-explorer-left

Janpot · 2023-12-14T14:41:19Z

packages/toolpad-app/src/components/EditableText.tsx

-        if (readOnly) {
-          return;
-        }
+        invariant(!readOnly, 'Readonly input should be disabled');


There's another instance in this file where this is happening

Janpot · 2023-12-14T14:43:01Z

packages/toolpad-app/src/toolpad/AppEditor/QueryIcon.tsx

@@ -29,25 +41,16 @@ export default function QueryIcon({ id: iconId, mode, sx }: QueryIconProps) {
  const ModeIcon = modeIconMap.get(mode ?? '');

  return (
-    <div style={{ display: 'flex' }}>
+    <div style={{ display: 'flex', gap: DataSourceIcon ? 0.5 : 0 }}>


Suggested change

<div style={{ display: 'flex', gap: DataSourceIcon ? 0.5 : 0 }}>

<div style={{ display: 'flex', gap: 0.5 }}>

The gap has no effect if there is only one child

…er-left

…olpad into query-explorer-left

bharatkashyap added 30 commits June 28, 2023 20:25

WIP page structure explorer

8ab8842

WIP add page structure explorer

26b3132

v1 page structure explorer

f37f538

don't crash on renderItem node

aeedf77

Merge branch 'master' into page-tree-view

f7b6dfa

Merge branch 'master' into page-tree-view

46b0059

Fix: Make side panel leaner

587d5b3

Fix: better nested element expansion, delete on backspace

108ff97

Merge branch 'master' of github.com:mui/mui-toolpad into page-tree-view

b76a300

Fix: accidental commit

72a8f2b

[WIP] Extract normalize function

964cc01

Fix: Remove unnecessary ternary through type check

aad5904

Extract delete functions for reusability

34ad45c

Merge branch 'master' of github.com:mui/mui-toolpad into page-tree-view

ae60aa1

Merge branch 'master' into page-tree-view

f3d2109

Revert "[WIP] Extract normalize function"

37e3691

This reverts commit 964cc01.

Revert "Extract delete functions for reusability"

02fa34e

This reverts commit 34ad45c.

only make call to appStateApi, parallel effort ongoing

b37715b

Merge branch 'master' of github.com:mui/mui-toolpad into page-tree-view

f1b5705

fix: better scrollbars

19dafdd

WIP add query explorer

5981ab6

Merge branch 'master' of github.com:mui/mui-toolpad into query-explor…

ee1c849

…er-left

Add react-draggable

af38cf1

Allow displaying enabled datasources

d11482b

Create a draggable Debugger

b93b7d9

Remove conflict

9d579af

Move state up to share between QueryPanel and Sidebar

5c6df9d

Denser UI

f9d3943

Add renaming for queries

a7f3e4e

Reorganise

4c78ce9

Janpot reviewed Dec 14, 2023

View reviewed changes

bharatkashyap added 3 commits December 14, 2023 14:25

No need to export

94907c1

Merge branch 'query-explorer-left' of github.com:bharatkashyap/mui-to…

6cbc9cd

…olpad into query-explorer-left

Merge branch 'master' into query-explorer-left

7b6b40c

Janpot reviewed Dec 14, 2023

View reviewed changes

Jan review

0fc2f75

Janpot approved these changes Dec 14, 2023

View reviewed changes

bharatkashyap added 4 commits December 14, 2023 19:00

Jan review

77c4e1f

Missed this when changing the way queries are created

c7cfa94

Merge branch 'query-explorer-left' of github.com:bharatkashyap/mui-to…

9d7d751

…olpad into query-explorer-left

Not needed

c9b61b1

Janpot reviewed Dec 14, 2023

View reviewed changes

Review tweaks

fa4c874

bharatkashyap mentioned this pull request Dec 14, 2023

Query panel improvements #2997

Open

5 tasks

bharatkashyap added 3 commits December 15, 2023 19:07

Fix renaming bug

27301b2

fix: Lint

48866d0

Merge branch 'master' into query-explorer-left

af5ee35

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 18, 2023

bharatkashyap added 3 commits December 19, 2023 12:46

Merge branch 'master' of github.com:mui/mui-toolpad into query-explor…

072b17f

…er-left

fix: Merge

f118367

Merge branch 'query-explorer-left' of github.com:bharatkashyap/mui-to…

a4c2a7f

…olpad into query-explorer-left

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 19, 2023

bharatkashyap added 3 commits December 19, 2023 14:37

fix: Merge

f68034f

Merge branch 'master' into query-explorer-left

a1dda58

Merge branch 'master' into query-explorer-left

473747f

bharatkashyap enabled auto-merge (squash) December 20, 2023 15:34

bharatkashyap merged commit 04a1e57 into mui:master Dec 20, 2023
2 checks passed

oliviertassinari mentioned this pull request Jul 14, 2024

[material-ui] Improve density for components mui/material-ui#41579

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new query panel #2393

Add a new query panel #2393

bharatkashyap commented Jul 30, 2023 •

edited

Loading

bharatkashyap commented Dec 13, 2023

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023 •

edited

Loading

Janpot left a comment

Janpot Dec 14, 2023

bharatkashyap Dec 14, 2023

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023

bharatkashyap Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

bharatkashyap Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

Janpot Dec 14, 2023

bharatkashyap Dec 14, 2023

Janpot Dec 14, 2023

bharatkashyap Dec 14, 2023

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023

Janpot Dec 14, 2023

	export const COMPONENT_CATALOG_WIDTH_COLLAPSED = 40;
	const COMPONENT_CATALOG_WIDTH_COLLAPSED = 40;

	if (readOnly) {
	invariant(!readOnly, 'Readonly input should be disabled')

	'& .MuiInputLabel-root': { fontSize: 12 },
	[`& .${inputLabelClasses.root}`]: { fontSize: 12 },

		@@ -18,6 +18,8 @@ export const TOOLPAD_BRIDGE_GLOBAL = '__TOOLPAD_BRIDGE__';

		export const VERSION_CHECK_INTERVAL = 1000 * 60 * 10;

		export const PAGE_PANEL_WIDTH = 250;

	<div style={{ display: 'flex', gap: DataSourceIcon ? 0.5 : 0 }}>
	<div style={{ display: 'flex', gap: 0.5 }}>

Add a new query panel #2393

Add a new query panel #2393

Conversation

bharatkashyap commented Jul 30, 2023 • edited Loading

TODO

bharatkashyap commented Dec 13, 2023

Janpot Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Janpot Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Janpot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janpot Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharatkashyap commented Jul 30, 2023 •

edited

Loading

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023 •

edited

Loading

Janpot Dec 14, 2023 •

edited

Loading