Skip to content

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Oct 21, 2020

PR sequence

This PR is part of a set of 3 PRs that implements the Credentials flyout form + functionality. I broke it up into PRs in the 400-800 line range so as not to open a 1700~ line PR 😬

List of all PRs (may be helpful for obtaining more context / QAing the entire final feature)

Summary

This PR achieves:

  • Adding FlashMessages to the credentials view as well as to the flyout.
    • Currently only the delete message is testable. Update/Create messages as well as in-flyout error messages will not be testable until the 3rd PR.
  • Stubbing out a bare bones flyout view (primarily header & footer). The body form & logic will be added in subsequent PRs.

Screencaps

screencap

QA

  • Flyout testing
    • Clicking "Create a Key" should open the flyout
    • Clicking the pencil icon on a key should open the flyout
    • Clicking the flyout save button should send a window alert
    • Clicking the flyout 'Close' button or outside the flyout should close the flyout
  • Flash Messages testing
    • Deleting a key should show a success message
    • Deleting a key and then opening the flyout should clear flash messages

Checklist

JasonStoltz and others added 4 commits October 21, 2020 11:26
+ split out child sub components for easier testing/reading
- mostly just DELETE_MESSAGE currently, since that's what's already wired up
- CREATE_MESSAGE and UPDATE_MESSAGE will be used in an upcoming commit

+ adds FlashMessages in flyout, which will show returned form errors from the API
e.g. deletion success messages

+ incidental linting/cleanup
@cee-chen cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Oct 21, 2020
@cee-chen cee-chen requested review from a team and JasonStoltz October 21, 2020 19:51
@cee-chen
Copy link
Contributor Author

cee-chen commented Oct 21, 2020

@JasonStoltz highlighting 8211f17 - I'm going to be splitting a ton of the flyout/form into sub-components as part of my PRs because I strongly think it makes the code easier to read and easier to write tests for (vs a single massive test file). In particular IMO, this definitely pays off when unit testing individual form components (see PR 3).

Let me know what you think!

Copy link
Member

@JasonStoltz JasonStoltz left a comment

Choose a reason for hiding this comment

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

LGTM.

import { i18n } from '@kbn/i18n';

import { SetAppSearchChrome as SetPageChrome } from '../../../shared/kibana_chrome';
import { FlashMessages } from '../../../shared/flash_messages';
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, guess we missed these previously.

@cee-chen cee-chen merged commit c9d4dc3 into elastic:master Oct 22, 2020
@cee-chen cee-chen deleted the credentials-flyout-1 branch October 22, 2020 18:06
cee-chen pushed a commit that referenced this pull request Oct 22, 2020
…out (#81391) (#81517)

* Added an empty Flyout

* Refactor CredentialsFlyout to its own component folder
+ split out child sub components for easier testing/reading

* Add initial FlashMessages setup

- mostly just DELETE_MESSAGE currently, since that's what's already wired up
- CREATE_MESSAGE and UPDATE_MESSAGE will be used in an upcoming commit

+ adds FlashMessages in flyout, which will show returned form errors from the API

* Fix flash messages appearing on flyout open
e.g. deletion success messages

+ incidental linting/cleanup

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>

Co-authored-by: Jason Stoltzfus <jastoltz24@gmail.com>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
enterpriseSearch 417 421 +4

async chunks size

id before after diff
enterpriseSearch 642.7KB 647.1KB +4.4KB

History

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.11.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants