-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[editor] Run Prompt Button UI #639
Conversation
# [editor] Prompt Output Bar Placeholder/starting point for the Output bar. In the future, we'll add metrics such as token usage to this component. <img width="998" alt="Screenshot 2023-12-27 at 11 26 16 AM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/29af67cc-9856-4b57-a7b5-74e0693c5089">
# [editor] Run Prompt Button UI Adding the Run prompt button to the UI; will connect the callbacks/server request in subsequent PR <img width="1061" alt="Screenshot 2023-12-27 at 11 28 23 AM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/4bf00f65-8983-4353-8e4d-020cca8840ca"> <img width="1050" alt="Screenshot 2023-12-27 at 11 28 31 AM" src="https://github.com/lastmile-ai/aiconfig/assets/5060851/56f64064-1333-43e0-b666-eb2c616991ab">
)} | ||
</Tabs> | ||
</Container> | ||
<RunPromptButton prompt={prompt} size="full" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd prefer to have this in the top right of the prompt-input area (not bottom of the settings), similar to what we have in the web app. Not a blocker for MVP, just commenting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, the current placement is a bit far from the input especially for large inputs/outputs. I was thinking it might be better to have it directly across from bottom-right of the input when the action bar is collapsed, otherwise top right of the settings. In any case, this is currently matching designs in https://www.figma.com/file/7YVa6KpEDgptj8DiAc5BaU/AiConfig-Editor?type=design&node-id=264-10830&mode=design&t=vIs81NxFIjhv8BbK-0 so we'll want to handle design/style updates after other things are more complete
cc @zakariassoul for visibility here as well
Note that figma designs have the icon and 'Run' context left-aligned in the full-width button. I couldn't get that layout within a few minutes, so leaving as-is for now. IMO centred does look fine as well. We can update this with style/UI pass later |
<ActionIcon | ||
size="sm" | ||
onClick={() => setIsExpanded(false)} | ||
mt="0.5em" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think having an "x" button would be more intuitive. Also having it inline horizontally with the tabs instead of ontop would look cleaner. Again not a blocker for MVP
<Flex direction="column" justify="space-between" h="100%"> | ||
<Flex direction="row" justify="center" mt="0.5em"> | ||
<ActionIcon size="sm" onClick={() => setIsExpanded(true)}> | ||
<IconClearAll /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry noob question, can you show screenshot and point which button is this from the UI? It's not clear to me right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
borderBottomLeftRadius: 0, | ||
borderTopLeftRadius: 0, | ||
borderTopRightRadius: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we only defining 3 of the corners? Also is 0 the default and not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want the bottom right to have radius to match the card border. Default is 0.25em, which already matches the prompt card border radius
import { memo } from "react"; | ||
|
||
type Props = { | ||
prompt: ClientPrompt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used this diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, but will be used in subsequent diff
const { classes } = useStyles(); | ||
return ( | ||
<Button | ||
onClick={() => {}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, we'll connect next diff, cool. Just a heads up right now I dont think the backend endpoint supports params
argument yet. I have diff for that but need to do some rebasing first: #613
@@ -11,11 +11,21 @@ type Props = { | |||
onUpdateModelSettings: (settings: Record<string, unknown>) => void; | |||
}; | |||
|
|||
const useStyles = createStyles(() => ({ | |||
settingsContainer: { | |||
maxHeight: "400px", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should we use something else other than px? Ex: %
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, but just doing this for now and we can update later with our UI/style pass through
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any blocking changes, mostly just questions to better understand, no blockers
# [editor] Add MantineProvider to Unblock Styling Work Adding the [MantineProvider](https://mantine.dev/theming/mantine-provider/) around the editor to allow customizing theme/styles from the root component. This will allow differentiation between local editor and gradio ## Testing: - Load editor without issue - Temporarily set theme={{primaryColor: "red"}} and see the correct changes --- Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/lastmile-ai/aiconfig/pull/640). * __->__ #640 * #639 * #638
[editor] Run Prompt Button UI
[editor] Run Prompt Button UI
Adding the Run prompt button to the UI; will connect the callbacks/server request in subsequent PR
Stack created with Sapling. Best reviewed with ReviewStack.