Lookup field implementation #505
Conversation
There was a problem hiding this comment.
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
featandfixwill end up in the changelog. Ideally, we should have only a single commit similar tofeat: added lookup fieldsfor 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
gosiexon-zen
force-pushed
the
mbien/lookup-field
branch
from
a25a358
to
afbaee1
Compare
There was a problem hiding this comment.
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.
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.
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.
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.
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.
ok, I changed that
| setOptions([]); | ||
| } | ||
| } catch (error) { | ||
| return error; |
There was a problem hiding this comment.
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.
What is this return for? I don't think we need it
There was a problem hiding this comment.
There is warning that not all the statements return a value
There was a problem hiding this comment.
There is warning that not all the statements return a value
There was a problem hiding this comment.
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.
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.
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.
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
gosiexon-zen
force-pushed
the
mbien/lookup-field
branch
2 times, most recently
from
9068534
to
e3a0bdc
Compare
chore: updated translations
gosiexon-zen
force-pushed
the
mbien/lookup-field
branch
from
85c7fa7
to
7e3dcbd
Compare
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