-
Notifications
You must be signed in to change notification settings - Fork 2
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: autocomplete data fields in the editor #4062
base: main
Are you sure you want to change the base?
Conversation
…ss/data-field-autocomplete
return formattedOption; | ||
}} | ||
renderOption={renderOptions} | ||
freeSolo |
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.
Explicitly passing the freeSolo
prop is a bit strange because it's already handled in AutocompleteProps<string, false, false, true, "div">
, but explicitly passing here seems to be the Stack Overflow consensus for how to clear up a console error related to isOptionEqualToValue
which assumes "new" options will never be added/present
id="data-field-autocomplete" | ||
key="data-field-autocomplete" | ||
placeholder="Data field" | ||
required={Boolean(props.required)} |
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.
required
prop will simply trigger browser default error handling which is quite consistent with current editor inputs !
Gov UK-looking error wrappers for autocomplete in the editor are larger future thing to revisit (eg would we simply wrap in our existing wrapper or want to account for error
-related props here?)
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 is what I would have gone for here as well! Currently we have a mix but this works great. We can revisit this when taking another design look at this modal alongside labels and layout.
backgroundColor: theme.palette.background.paper, | ||
[`& .${outlinedInputClasses.root}, input`]: { | ||
cursor: "pointer", | ||
// TODO extract as `format="data"` prop more like `Input` ? |
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 TODO
is good to note, but I don't think we need to pick it up until there's another use case/iteration of the autocomplete input - any objections?
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.
Yep good point - I don't think we really need a format
prop here right now - a TODO works for me!
@@ -92,6 +94,8 @@ function ListComponent(props: Props) { | |||
validateOnChange: false, | |||
}); | |||
|
|||
const dataFieldSchema = useStore().getFlowSchema()?.nodes; |
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.
List
, Page
& MapAndLabel
components have their own pre-existing concepts of "schemas", so the store function is more explicitly assigned to dataFieldSchema
in these cases
|
||
// TODO align to (reuse?) data facets from search | ||
if (node.data?.output) nodes.add(node.data.output); | ||
if (node.data?.dataFieldBoundary) nodes.add(node.data.dataFieldBoundary); |
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.
await page.getByPlaceholder("Data Field").fill(options?.[0] || ""); | ||
await page | ||
.getByRole("combobox", { name: "Data field" }) | ||
.fill(options?.[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.
required
data field are not being correctly populated - I've tried a couple iterations here, but still not landed on the right solution.
Opening for wider review in the meantime and any pointers very welcome!
Here's what I've tried so far:
- The placeholder text has changed from
"Data Field"
to"Data field"
, butgetByPlaceholder()
will now match more than one element (MUI Autocompletes have many nested elements)
- So,
"getByRole("combobox", { name: "Data field" })
should exclusively be matching theinput
element - On the first component, there will be no flow schema or suggestions yet and the
popupIcon
arrow is disabled, so there's no dropdown of options to.click()
or open/expand - but I'm guessing I'm still using the wrong combo of.fill
/.click
here to mimic typing input ??
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 is working for me locally - 5114475
Running on CI now 🤞
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.
It failed on some still - to be fair I didn't run them all locally just the first few. Will pick this up again but I think adding the .click()
is what we're after.
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.
Appreciate second eyes on this - it's a tedious tricky one! Once one node with a data field is successfully added, then future ones will have suggested schema options - so might require combination of fill
& click
?
|
||
const schema = useStore().getFlowSchema()?.options; | ||
const initialOptions: Option[] | undefined = formik.initialValues.options || formik.initialValues.groupedOptions?.map((group: Group<Option>) => group.children)?.flat(); | ||
const initialOptionVals = initialOptions?.map((option) => option.data?.val); |
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 is a bit of a lazy rebase right now while the exclusive-or work is still in-progress - I think initialOptions
should be able to be simplified/derived from exclusiveOptions
or nonExclusiveOptions
once those are handling "grouped" options
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.
Great! Zero issues - all working as expected, code was clear and easy to follow.
Appreciate the detailed PR description and thought that's gone into this - great feature 😄
import InputRow from "ui/shared/InputRow"; | ||
|
||
interface Props { | ||
schema?: string[]; |
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.
Could be have getStore().getFlowSchema()?.nodes
be the default value for this prop so that it doesn't need to be defined in many components?
It also then feel more explicit where there's an exception (e.g. for options).
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.
💡 Good suggestion! Will aim to push this change up this afternoon
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 satisfying one to implement ✂️ thanks again ! 867a6c2
backgroundColor: theme.palette.background.paper, | ||
[`& .${outlinedInputClasses.root}, input`]: { | ||
cursor: "pointer", | ||
// TODO extract as `format="data"` prop more like `Input` ? |
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.
Yep good point - I don't think we really need a format
prop here right now - a TODO works for me!
id="data-field-autocomplete" | ||
key="data-field-autocomplete" | ||
placeholder="Data field" | ||
required={Boolean(props.required)} |
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 is what I would have gone for here as well! Currently we have a mix but this works great. We can revisit this when taking another design look at this modal alongside labels and layout.
@@ -63,3 +64,21 @@ export const getPreviouslySubmittedData = ({ | |||
|
|||
return data; | |||
}; | |||
|
|||
export const getOptionsSchemaByFn = (fn?: string, defaultOptionsSchema?: string[], initialOptions?: (string | undefined)[]) => { |
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.
Super clear and easy to follow 👍
if (node.data?.fn !== "flag") nodes.add(node.data.fn) | ||
}; | ||
|
||
// TODO align to (reuse?) data facets from search |
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.
Great comment!
We could probably use DATA_FACETS
here (filtered to strings only) and then use this to get the data values from each node.
However, I think this is actually a more complex solution - there's a very different use case here and we'll have just as many exceptions and clauses. This works really well for now - we just need to make sure that we keep data values for new components, this function, and DATA_FACETS
in sync.
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.
Yep - fn
only basically felt like right "first" go at this since other DATA_FACETS
like FileUploadAndLabel individual file fns are getting "overwritten" by ODP Schema enum suggestions anyways ! So it's a good question of whether or not we actually always want this to be perfectly in sync !
P.S. Happy to have a crack at Playwright later today and push a commit here 👍 |
710156f
to
ec9c832
Compare
Key changes:
getFlowSchema
which gets all the existing data fields (fn
&val
) for a given flow split into "nodes" (what you can click on in the graph) and "options" (Question & Checklist "options")<DataFieldAutocomplete />
component which we can plug in across the editor like other input rowsQuestion
&Checklist
are most complexfn
suggestions reflect flow nodes, optionsval
suggestions reflect other options onlyfn
is any ofproposal.projectType
,property.type
, orapplication.type
, we'll override default suggested options in favor of ODP Schema enum valuesinitialValues
are additionally supported, as well as option to "Add.." new values, to ensure editors maintain flexibility and are able to use and experiment with data values outside of the current schema releaseFileUpload
&FileUploadAndLabel
FileType
enums; sameinitialValues
& "Add.." new values handling as aboveTodos:
https://trello.com/c/6OkLrUNE/2668-autofill-passport-variables-when-creating-nodes-in-the-editor