-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add UI notifier to indicate secret fields and to remember / reenter values #80657
Add UI notifier to indicate secret fields and to remember / reenter values #80657
Conversation
@gchaps can you please review the screenshots in the descriptions? It contains the suggestions you've proposed here: #78476 (comment). Note that I did change the wording a bit for IBM resilient ( @mdefazio can you please review the screenshots in the description from a design perspective? Just to confirm it turned out the way you were hoping it would. |
I don't think we need the green color on the text. I vote for the default EUI color text. It's the only sentence there so it's scannable enough IMO. Other than that, all looks good. |
Thanks @mdefazio! Just to confirm, you're referring to the create forms where the message is using |
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
Yes. Sorry for the confusion.
|
Here are my comments: Create email connector
Edit email connector
Create IBM Resilient connector
Edit IBM Resilient connector
Create Jira connector
Edit Jira connector
Create PagerDuty connector
Edit PagerDuty connector
Create ServiceNow connector
Edit ServiceNow connector
Create Slack connector
Edit Slack connector
Create Webhook connector
Edit Webhook connector
|
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.
LGTM pending suggested UX and wording updates!
Curious if there is a reason why some username and password fields are on a single row and some are on separate rows? Ex: Email connector vs ServiceNow connector?
@gchaps @mdefazio Thank you both for the feedback. I went ahead and updated the screenshots with the latest changes. @gchaps A few things I didn't change yet, see below:
How about something like
I will echo this feedback to the team at our next design meeting and see what we can come up with. I'll be happy to circle back and change that in a follow up PR (since that text is already in use today). |
@ymao1 thanks for the review!
Good catch, no reason at all that I can think of. I'm guessing it's just an inconsistency but worth opening an issue to get it fixed. |
@mikecote Good idea to talk to design about the placeholder text. |
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.
Just a comment for now, after looking at the screen shots, full review in a bit.
I like it! :-)
This is the first time I noticed the hasAuth
on the email connector, and happened to remember when that was added, that webhook has the same sort of thing - the user/password are optional. I thought we opened an issue to address that, but I'm not seeing one. It would presumably need the same sort of hasAuth
switch. We could extend the messaging there for now, indicating somehow they are optional, but if you did enter one before, you will need to enter it again - or whatever. Or leave it as it is in this PR, I think all the webhook calls I've seen in practice so far do require a user/pass, and we can address later.
Do you happen to know if we do have an issue on the webhook hasAuth
switch? I'm not seeing one ...
I don't recall seeing one and just did a search myself and turned out negative. I'm thinking of leaving it as is in this PR and opening an issue to add the |
@gchaps thank you, I went ahead and applied the change and updated the screenshots. |
I have opened this issue to fix the Webhook connector to use the |
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.
LGTM
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
…alues (elastic#80657) * Initial work * Add messaging to Jira connector * Add messaging to PagerDuty connector * Add messaging to Email connector * Add messaging to ServiceNow connector * Add messaging to Webhook connector * Add unit tests * Fix jest test * Apply design feedback * Apply copy feedback * Fix failing test * Fix connector descriptions for jira and resilient Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…alues (#80657) (#81371) * Initial work * Add messaging to Jira connector * Add messaging to PagerDuty connector * Add messaging to Email connector * Add messaging to ServiceNow connector * Add messaging to Webhook connector * Add unit tests * Fix jest test * Apply design feedback * Apply copy feedback * Fix failing test * Fix connector descriptions for jira and resilient Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* master: (63 commits) [KP] Fix Headers timeout issue (elastic#81140) [ML] Functional tests - stabilize typing with checks service method (elastic#81338) tabify - support docs (elastic#80351) [Security Solution][Detections] Look-back time logic fix (elastic#81383) [Workplace Search] Add top-level tests for Groups (elastic#81215) [Fleet] Fix agent action observable for long polling (elastic#81376) [Maps] fix feature tooltip remains open when zoom level change hides layer (elastic#81373) skip flaky suite (elastic#78689) chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies (elastic#81357) Ensure some data is returned (elastic#81375) Change dumb-init to tini (elastic#81126) [Reporting/Tech Debt] Convert PdfMaker class to TypeScript (elastic#81242) Use Storybook Controls instead of Knobs (elastic#80705) [junit] make sure that report paths are unique (elastic#81255) bump elastic/elasticsearch-js version to 7.10.0-rc1 (elastic#81288) run ssl tests on CI (elastic#81320) Fix alert defaults (elastic#81207) [ML] DF Analytics wizard: ensure user can set mml manually or select to use given estimate (elastic#81078) Add UI notifier to indicate secret fields and to remember / reenter values (elastic#80657) [Monitoring] Use async/await (elastic#81200) ...
Resolves #78476
In this PR, I'm adding a message above encrypted fields to indicate to the user they must remember the values when creating a connector as well as explaining why the values are empty when editing the connector.
Screenshots
Create email connector
Edit email connector
Create IBM Resilient connector
Edit IBM Resilient connector
Create Jira connector
Edit Jira connector
Create PagerDuty connector
Edit PagerDuty connector
Create ServiceNow connector
Edit ServiceNow connector
Create Slack connector
Edit Slack connector
Create Webhook connector
Edit Webhook connector
Note for docs
This PR changes the connector create / edit flyouts so screenshots will need to be updated for most (minus server log, index action). The user docs could also have the same messaging about the encrypted values. Copy: #78476 (comment).