[editor] Support Deleting Prompt #665

rholinshead · 2023-12-29T02:33:43Z

[editor] Support Deleting Prompt

Add the UI side for deleting a prompt, with placeholder to call the server endpoint once it's implemented

Screen.Recording.2023-12-28.at.9.33.24.PM.mov

Stack created with Sapling. Best reviewed with ReviewStack.

rossdanlm · 2023-12-29T15:18:51Z

placeholder to call the server endpoint once it's implemented

I thought Jonathan added the delete_prompt endpoint on the backend here? #595

rossdanlm · 2023-12-29T15:22:01Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+import {
+  Container,
+  Group,
+  Button,
+  createStyles,
+  Stack,
+  Flex,
+} from "@mantine/core";


Based on my understanding from your response in #640 (comment), this is just fancy colors around base components right?

For mantine in general? Think of it more like XDS - it's a component library that gives a lot of out-of-the-box functionality and common styles that can be easily updated across the components

rossdanlm · 2023-12-29T15:22:45Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+        id: promptId,
+      };
+
+      dispatch(action);


why not inline action to stay consistent with #662 ?

rossdanlm · 2023-12-29T15:27:24Z

python/src/aiconfig/editor/client/src/components/aiconfigReducer.ts

+    case "DELETE_PROMPT": {
+      return {
+        ...state,
+        prompts: state.prompts.filter((prompt) => prompt._ui.id !== action.id),


Why do we need to nest this extra _ui.id field instead of using id directly in

aiconfig/python/src/aiconfig/editor/client/src/shared/types.ts

Lines 12 to 15 in ca59d2c

export type ClientPrompt = Prompt & {

_ui: {

id: string;

};

I think it makes it really clear that there's a UI-only distinction between ClientPrompt and Prompt and makes it clearer to reason about for conversion between the two where needed (we should remove UI-only state like this so it's not propagated to the config on disk)

# [editor] Model Selector Implement the model selector and add a placeholder call to the future api endpoint for updating the model. https://github.com/lastmile-ai/aiconfig/assets/5060851/812f3a10-aaac-4ec3-9f1f-4d21ebc99e9f

rossdanlm · 2023-12-29T15:31:47Z

This is also super nit and not a blocker, but noticed from the Figma file that there's no clear way of deleting a cell. I personally feel like having it on the left is unintuitive (since most systems have action menu on the top right), but yea not a blocker, just wanted to point out it seems strange to me

rossdanlm · 2023-12-29T15:32:41Z

python/src/aiconfig/editor/client/src/components/EditorContainer.tsx

+                <PromptMenuButton
+                  promptId={prompt._ui.id}
+                  onDeletePrompt={() => onDeletePrompt(prompt._ui.id)}
+                />


Oh yea left comment (#665 (comment)) how I said this feels a bit weird it being on the left instead of right, but yea not a big deal

But also why not put this within the prompt container?

Just a design thing, it's not in the figma designs so just went with how it is in workbooks right now. We can update later

rossdanlm · 2023-12-29T15:34:21Z

python/src/aiconfig/editor/client/src/components/prompt/PromptContainer.tsx

@@ -107,7 +107,7 @@ export default memo(function PromptContainer({
  const { classes } = useStyles();

  return (
-    <Flex justify="space-between" mt="md">


Very quickly, what does the mt field mean in typescript components?

It's margin-top in supported mantine components

rossdanlm · 2023-12-29T15:35:21Z

python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

+    marginLeft: -8,
+    [theme.fn.smallerThan("sm")]: {
+      float: "left",
+      marginTop: -2,


Feels kind of strange to use negative margins (and kind of related to my question about why not put this within the prompt container itself), but yea not a big deal

rossdanlm · 2023-12-29T15:36:41Z

python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

+      zIndex: 99,
+      "&:hover": {
+        backgroundColor:
+          theme.colorScheme === "dark"


Very cool, but is this not supported automatically? Ex: at FB we have color.primary, color.emphasized, etc

Ya, this was copied over from workbooks as well. I can double-check if it's even needed. Mantine does have auto-styling already based on theme so not sure what this was intended for there

rossdanlm · 2023-12-29T15:39:43Z

python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

+  const { classes } = useStyles();
+
+  return (
+    <Menu position="bottom-end">


Nit: could we wrap this inside of a more visible border/box container? It feels now a bit like the Delete Prompt dropdown item is just sort of floating below without firmly being connected to the three vertical dots icon

Uh, not sure off the top of my head how to do that. But since this placement/UI isn't final anyway I'll hold off on anything for this

rholinshead · 2023-12-29T15:39:53Z

placeholder to call the server endpoint once it's implemented

I thought Jonathan added the delete_prompt endpoint on the backend here? #595

Ah, right, let me hook it up and update

rossdanlm · 2023-12-29T15:41:01Z

python/src/aiconfig/editor/client/src/components/prompt/PromptMenuButton.tsx

+        </Button>
+      </Menu.Target>
+
+      <Menu.Dropdown>


Just to be clear, is Menu.Dropdown automatically maintaining it's state to be visible or not based on whether you've clicked the Menu.Target component?

rossdanlm

lgtm, just some minor nits, can be fixed forward

# [editor] Support Deleting Prompt Add the UI side for deleting a prompt, with placeholder to call the server endpoint once it's implemented https://github.com/lastmile-ai/aiconfig/assets/5060851/b3e682dd-a3ed-45c1-b153-b4b6e2094616

rholinshead · 2023-12-29T15:54:14Z

Made changes:

connect to the endpoint to delete the prompt in the config server-side
remove unnecessary styles (mainly "sm" screen size for button)

[editor] Run Prompt (non-streaming) # [editor] Run Prompt (non-streaming) With non-streaming /run endpoint properly returning the response now, we can update the client config with the resulting output. We can also leverage the client-side `_ui` state for maintaining 'isRunning' state to show a spinner / disable the execute button for now (TODO: we may need to handle canceling execution as well). https://github.com/lastmile-ai/aiconfig/assets/5060851/67fa793a-9a60-4285-a7d1-69fff633d6a6 --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/667). * __->__ #667 * #666 * #665 * #662

rholinshead mentioned this pull request Dec 29, 2023

[editor] Model Selector #662

Merged

rholinshead marked this pull request as ready for review December 29, 2023 02:36

rholinshead requested review from saqadri, suyoglastmileai, Ankush-lastmile, jonathanlastmileai and rossdanlm as code owners December 29, 2023 02:36

rholinshead force-pushed the pr665 branch from b680f62 to 75a02df Compare December 29, 2023 03:06

rholinshead mentioned this pull request Dec 29, 2023

[editor] Use Prompt ID Instead of Index for Client-side State Actions #666

Merged

rholinshead force-pushed the pr665 branch from 75a02df to 41a0063 Compare December 29, 2023 03:09

rholinshead mentioned this pull request Dec 29, 2023

[editor] Run Prompt (non-streaming) #667

Merged

rossdanlm reviewed Dec 29, 2023

View reviewed changes

[editor] Model Selector

2bda80c

# [editor] Model Selector Implement the model selector and add a placeholder call to the future api endpoint for updating the model. https://github.com/lastmile-ai/aiconfig/assets/5060851/812f3a10-aaac-4ec3-9f1f-4d21ebc99e9f

rossdanlm reviewed Dec 29, 2023

View reviewed changes

rossdanlm approved these changes Dec 29, 2023

View reviewed changes

[editor] Support Deleting Prompt

9b7bee7

# [editor] Support Deleting Prompt Add the UI side for deleting a prompt, with placeholder to call the server endpoint once it's implemented https://github.com/lastmile-ai/aiconfig/assets/5060851/b3e682dd-a3ed-45c1-b153-b4b6e2094616

rholinshead force-pushed the pr665 branch from 41a0063 to 9b7bee7 Compare December 29, 2023 15:52

rholinshead merged commit 8c35024 into main Dec 29, 2023
2 checks passed

rossdanlm deleted the pr665 branch December 29, 2023 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[editor] Support Deleting Prompt #665

[editor] Support Deleting Prompt #665

rholinshead commented Dec 29, 2023 •

edited

Loading

rossdanlm commented Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading

rossdanlm commented Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rholinshead commented Dec 29, 2023

rossdanlm Dec 29, 2023

rholinshead Dec 29, 2023

rossdanlm left a comment

rholinshead commented Dec 29, 2023

[editor] Support Deleting Prompt #665

[editor] Support Deleting Prompt #665

Conversation

rholinshead commented Dec 29, 2023 • edited Loading

[editor] Support Deleting Prompt

rossdanlm commented Dec 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

rholinshead Dec 29, 2023 • edited Loading

Choose a reason for hiding this comment

rossdanlm commented Dec 29, 2023

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

rholinshead commented Dec 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossdanlm left a comment

Choose a reason for hiding this comment

rholinshead commented Dec 29, 2023

rholinshead commented Dec 29, 2023 •

edited

Loading

rossdanlm Dec 29, 2023 •

edited

Loading

rholinshead Dec 29, 2023 •

edited

Loading