Skip to content
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

[Runtime field editor] Component integration test for preview #106410

Conversation

sebelga
Copy link
Contributor

@sebelga sebelga commented Jul 21, 2021

This PR adds the component integration tests for the preview panel inside the runtime field editor. While adding the tests I found a few issues around timings and inconsistencies when updating some states (I found them while doing QA with throttling on "Slow 3G"). Specially when switching between cluster data and loading a custom doc ID.

I am confident that the code is more robust now and the tests should give us a very high level of confidence against future regressions. 👍

Preview for concrete fields

I've also enabled the preview for concrete field when the "Set format" is turned on.

Screenshot 2021-07-27 at 11 03 51

@sebelga sebelga force-pushed the runtime-field-editor/component-integration-tests branch from e794b0a to 04084c7 Compare July 26, 2021 16:57
}
}, [currentDocument, isCustomId]);

useDebounce(
Copy link
Contributor Author

@sebelga sebelga Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the logic to fetch the document inside the preview context file to have a single place to look at for fetching document. This also allows us to keep the state when the doc navigation is unmounted (for example when showing the empty prompt)

@sebelga sebelga force-pushed the runtime-field-editor/component-integration-tests branch from 04084c7 to 937f2fa Compare July 27, 2021 09:34
@sebelga sebelga marked this pull request as ready for review July 27, 2021 09:55
@sebelga sebelga requested a review from a team as a code owner July 27, 2021 09:55
@sebelga sebelga requested review from mattkime and yuliacech July 27, 2021 09:56
previewCount === 0;
: name === null && format === null
? true
: false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this expression could be made more readable by splitting into several if/else statement with isEmptyPromptVisible being false by default? I find multiple ternary operators very complex to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You right, your suggestion will make it easier to follow. I'll make the change 👍

Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sebelga, great work! I think adding so many tests makes this feature way more robust and stable!
I followed the tests and tried out everything locally. The preview works as expected and I haven't noticed any bugs.

I'd like to comment on some UX that might be improved in a follow up work and are not blockers for this PR:

  • When the error is too long, it goes out of the callout, I like to use EUI's utility classes to fix this (eui-textBreakWord or eui-textBreakAll)

Screenshot 2021-07-28 at 13 58 16

  • Pinned fields are not preserved between a custom document ID and loading documents from the cluster
    pin

  • When the document changes, the field's preview is updating (which is indicated on the top of the flyout), but there is no indication for that at the field itself. It might be confusing when the value just suddenly changes, especially when other fields have already been loaded from the document (I tested this on 'Slow 3G')
    preview_updating

@sebelga
Copy link
Contributor Author

sebelga commented Jul 29, 2021

Thanks for the review @yuliacech! You made some great UX enhancement suggestions. I added them in the backlog and will address them as a separate chunk of work. 👍

@mattkime
Copy link
Contributor

mattkime commented Aug 1, 2021

src/plugins/index_pattern_field_editor/__jest__/client_integration/field_editor.test.tsx:78 - has 'TODO' as content - is this intentional?

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left just one comment which I'll let you address as you please. I really like how detailed these jest tests are. I must admit that I'm new to using act and testBed in tests so while I'm reviewing as best I can, someone else might be more rigorous. Thanks!

@mattkime
Copy link
Contributor

mattkime commented Aug 1, 2021

@elasticmachine merge upstream

…time-field-editor/component-integration-tests
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@sebelga
Copy link
Contributor Author

sebelga commented Aug 2, 2021

Thanks for the review @mattkime !

has 'TODO' as content - is this intentional?

Good catch, I removed the TODO as it has been covered. 👍

@sebelga sebelga merged commit 3954533 into elastic:runtime-field-editor/preview-field-workstream Aug 2, 2021
@sebelga sebelga deleted the runtime-field-editor/component-integration-tests branch August 2, 2021 13:32
@cjcenizal
Copy link
Contributor

@sebelga Could you please add labels to this PR, too?

@sebelga sebelga added Project:RuntimeFields Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Aug 4, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Project:RuntimeFields Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants