[LOOKUP_JOIN] Add new row to index (PART 1)#224567
Merged
sddonne merged 19 commits intoelastic:feature/lookup-join-uxfrom Jun 20, 2025
Merged
[LOOKUP_JOIN] Add new row to index (PART 1)#224567sddonne merged 19 commits intoelastic:feature/lookup-join-uxfrom
sddonne merged 19 commits intoelastic:feature/lookup-join-uxfrom
Conversation
darnautov
reviewed
Jun 20, 2025
src/platform/packages/private/kbn-index-editor/src/components/row_column_creator.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/private/kbn-index-editor/src/components/row_column_creator.tsx
Outdated
Show resolved
Hide resolved
| [columns, updateRow] | ||
| ); | ||
|
|
||
| const saveNewRow = async (event: React.FormEvent<HTMLFormElement>) => { |
Contributor
There was a problem hiding this comment.
I think it's worth using the useCallback here
| <EuiFlexGroup gutterSize="s" alignItems="center"> | ||
| <EuiFlexGroup | ||
| gutterSize="s" | ||
| tabIndex={0} |
Contributor
There was a problem hiding this comment.
do we need a tabIndex here?
Contributor
Author
There was a problem hiding this comment.
It's a suggestion from EUI about scrollable sections: https://eui.elastic.co/docs/utilities/scroll/
src/platform/packages/private/kbn-index-editor/src/components/row_column_creator.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/private/kbn-index-editor/src/components/row_column_creator.tsx
Outdated
Show resolved
Hide resolved
src/platform/packages/private/kbn-index-editor/src/components/row_column_creator.tsx
Outdated
Show resolved
Hide resolved
| autoFocus = false, | ||
| className = '', | ||
| }: ValueInputProps) => { | ||
| const [editValue, setEditValue] = useState(value); |
Contributor
There was a problem hiding this comment.
I don't think we should maintain a local state here, both value and onChange passed from the parent component
Suggested change
| const [editValue, setEditValue] = useState(value); |
Contributor
Author
There was a problem hiding this comment.
For now it's only there because of the onKeyDown = Enter check, that event does not have the target.value.
But it's also going to be needed if we are some validations in here.
src/platform/packages/private/kbn-index-editor/src/index_update_service.ts
Outdated
Show resolved
Hide resolved
darnautov
approved these changes
Jun 20, 2025
sddonne
merged commit 8959bb8
into
elastic:feature/lookup-join-ux
8 of 10 checks passed
Contributor
💔 Build Failed
Failed CI StepsHistory |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Pendings
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesIdentify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.