-
Notifications
You must be signed in to change notification settings - Fork 285
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
feat(datasets): add dataset edit UI and dataset metadata on create #4005
Conversation
> | ||
<Flex direction="row" justifyContent="end"> | ||
<Button | ||
disabled={!isDirty} |
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.
isDisabled prop doesn't actually disable button - only allow for clicking save when fields have changed, invalid will be caught by useForm rules
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.
Yeah I noticed that too the other day. We should fix in the component lib
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.
yeah both arize and phoenix are all over the place isDisabled / disable
import { css } from "@emotion/react"; | ||
|
||
export const metadataFieldWrapperCSS = css` | ||
.ac-field__field { |
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.
makes the json editor full width
@@ -41,10 +41,10 @@ export function DatasetsPageContent() { | |||
{}, | |||
{ | |||
fetchKey: fetchKey, | |||
fetchPolicy: "store-and-network", |
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.
makes table below refetch
@@ -0,0 +1,73 @@ | |||
import React, { startTransition, useCallback } from "react"; | |||
import { useMutation } from "react-relay"; |
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.
just moved out of DatasetActionMenu
|
||
type PatchDatasetParams = Omit<EditDatasetFormMutation$variables, "datasetId">; | ||
|
||
export function EditDatasetForm({ |
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.
Very minor nit and can be done as a follow-up but I think if organized well we can make a non relay connected form called DatasetForm and then have Edit and Create variants just be wrappers that have relay stuff and we can put that on pages. It might be worth doing just so we start down this pattern. LMK
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.
yup i like it, i was thinking the same when i was making this form, will put it as a follow up if it seems big
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 think it's probably worth just doing to we can de-couple form logic from relay logic. It seems like the right time to keep things DRY
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.
pretty easy refactor so doing it in this pr
title: "Dataset updated", | ||
message: `${row.original.name} has been successfully updated.`, | ||
}); | ||
refetch({}, { fetchPolicy: "store-and-network" }); |
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.
this totally works. But might make sense to just switch this stuff to relay refetching of fragments. makes things a bit more encapsulated.
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.
yeah i wanted to spread the fragment. Actually is this possible without changing mutations server side since datasets are queryable top level? i didn't consider that, i was thinking i needed the parent on the return of the Created / Edited dataset, not sure how flexible relay is with that... will give it a shot
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 think you might have to. Let's file a follow-up and do it. Best to make things more efficient as we go.
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.
yeah i'll file a follow up can't query datasets off the return
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.
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.
Nice one! I'll pull down tomorrow and approve
updated code block in form Screen.Recording.2024-07-25.at.10.46.59.AM.mov |
const [isHovered, setIsHovered] = useState(false); | ||
const isInvalid = validationState === "invalid"; | ||
return ( | ||
<Field {...fieldProps}> |
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.
went back and forth on adding this in here or making it outside of this component, let me know what you think @mikeldking
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 think it belongs here yeah
/** | ||
* Wrapper for code editor components (e.g. JSONEditor) that provides hover, focus, and validation state styles | ||
*/ | ||
export function CodeEditorFormWrapper({ |
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.
went with children here and generic CodeEditorFormWrapper (instead of specifically wrapping the json editor. Since in theory this can and should be applied to any code editor in a form (may or may not always be json editor).
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.
Thank you!
/** | ||
* Wrapper for code editor components (e.g. JSONEditor) that provides hover, focus, and validation state styles | ||
*/ | ||
export function CodeEditorFormWrapper({ |
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.
export function CodeEditorFormWrapper({ | |
export function CodeEditorFieldWrapper({ |
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.
Since it's really wrapping the Field, not the form
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.
makes sense
const [isHovered, setIsHovered] = useState(false); | ||
const isInvalid = validationState === "invalid"; | ||
return ( | ||
<Field {...fieldProps}> |
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 think it belongs here yeah
resolves #3938
Screen.Recording.2024-07-24.at.5.22.36.PM.mov