-
Notifications
You must be signed in to change notification settings - Fork 393
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
Lookup field implementation #505
Conversation
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 left some comments in the PR.
Other things to improve:
- I created a lookup-fields branch. Let's set it as a target branch for the PR and rebase the PR on top of it, so when the PR is ready we can merge it there and eventually iterate on the implementation before merging it on master and releasing a new version
- Let's rewrite and simplify the commit history. Every commit with
feat
andfix
will end up in the changelog. Ideally, we should have only a single commit similar tofeat: added lookup fields
for this PR - I don't like how the lookup field works, especially the fact that the user needs to manually delete the content to be able to search for a different string. I think we should align the behavior with the Lookup field in Support, but we can ask a feedback to design as well
a25a358
to
afbaee1
Compare
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 we still have some things to address, I left some comments
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 the implementation is ok, let's see if we can use the new organizations
from Curlybars. Otherwise, we can use the current implementation for testing.
Before approving the PR it would be nice to have that change for organizations
and also squash the changes to a single commit with the message feat: added support for lookup fields
const handleKeyDown = useCallback((event) => { | ||
if (event.key === "Enter") { | ||
event.preventDefault(); | ||
} | ||
}, []); |
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 do we need this? I don't think it is a good idea to override the default key bindings
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.
Because, when we hit enter in combobox we are submitting the whole form 🤷♀️ Not sure why it's only in this combobox, I've checked other fields also.
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 is the normal behavior of form inputs. If you press enter on any field (like subject, custom text input, custom date, cc field, ...) it will submit the form as well. The only fields where it is not happening are the WYSIWYG because the user needs to insert new lines and the other dropdowns because they don't have an input inside (we pass isEditable={false}
as a prop).
isAutocomplete | ||
placeholder={t( | ||
"new-request-form.lookup-field.placeholder", | ||
`Search ${label.toLowerCase()}` |
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 not the correct way to interpolate a string in the translation, we need pass the value as options as we are doing here. Reference: https://www.i18next.com/translation-function/interpolation
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.
ok, I changed that
setOptions([]); | ||
} | ||
} catch (error) { | ||
return error; |
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 doesn't make much sense to return the error, nobody is using that. Maybe it is better to log it to the console
} finally { | ||
setIsLoadingOptions(false); | ||
} | ||
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.
What is this return for? I don't think we need it
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 is warning that not all the statements return a value
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 is warning that not all the statements return a value
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.
Ok, there was before, maybe after splitting handle change it is not needed anymore
); | ||
|
||
const data = await response.json(); | ||
if (data !== 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.
Is there any case when the data can be undefined? I don't think it is possible but we would probably need to check if response.ok
is true instead
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.
ok I changed to check response.ok
const res = await fetch( | ||
`/api/v2/custom_objects/${customObjectKey}/records/${selectionValue}` | ||
); | ||
const { custom_object_record } = await res.json(); | ||
const newSelectedOption = { | ||
name: custom_object_record.name, | ||
value: custom_object_record.id, | ||
}; | ||
setSelectedOption(newSelectedOption); | ||
setInputValue(custom_object_record.name); |
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 is better to handle possible errors here:
- wrapping everything in a try catch
- checking if the response is ok before parsing it
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.
ok changed
9068534
to
e3a0bdc
Compare
chore: updated translations
85c7fa7
to
7e3dcbd
Compare
🎉 This PR is included in version 4.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Description
This code add support for Lookup fields for custom objects in the Copenhagen Theme v4.
Jira GG-3522
Screenshots
Screen.Recording.2024-08-08.at.17.31.52.mov
Checklist