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

Fixed adding an extra space character on selecting alert variable in action text fields #70028

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jun 26, 2020

Resolve #68208

New variables button and behavior:
img

@YulNaumenko YulNaumenko added bug Fixes for quality problems that affect the customer experience release_note:fix Feature:Alerting v8.0.0 Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.9.0 v7.8.1 labels Jun 26, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner June 26, 2020 00:49
@YulNaumenko YulNaumenko self-assigned this Jun 26, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

@gmmorris gmmorris self-requested a review June 26, 2020 14:57
Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

Made a comment for the email fix, but I think it's true for all of them.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

Concur w/Gidi that there seems to be something off here.

Trying it out locally, seems to set the field to the "{{variable}}" (no blanks) if the current field is empty, but if there's any content in the field, then sets it to " {{variable}}" (one preceding blank) if the current field is non-empty.

It would actually be nice if we could just insert the {{variable}} text at the insertion point (cursor) of the text field, if it has focus, so it would be inserted where you'd expect - not sure we have access to the insertion point tho. Worse case, seems like we should append it to the existing text.

I don't think there are any cases where we'd want to insert a space anywhere here, unless we do have access to the insertion point - if we do, we could insert a space before and/or after the {{variable}} if the characters before/after the insertion point aren't whitespace, but ... we could leave that for another issue/PR :-)

…pace-before-add-variable

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@YulNaumenko
Copy link
Contributor Author

Concur w/Gidi that there seems to be something off here.

Trying it out locally, seems to set the field to the "{{variable}}" (no blanks) if the current field is empty, but if there's any content in the field, then sets it to " {{variable}}" (one preceding blank) if the current field is non-empty.

It would actually be nice if we could just insert the {{variable}} text at the insertion point (cursor) of the text field, if it has focus, so it would be inserted where you'd expect - not sure we have access to the insertion point tho. Worse case, seems like we should append it to the existing text.

I don't think there are any cases where we'd want to insert a space anywhere here, unless we do have access to the insertion point - if we do, we could insert a space before and/or after the {{variable}} if the characters before/after the insertion point aren't whitespace, but ... we could leave that for another issue/PR :-)

I changed the approach for adding variables - introduced a components which allows to add variable by the cursor position :)

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, this is a much cleaner approach than what we had before 👍

Left a few small notes, but nothing blocking

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

code LGTM

code works as expected except for the case of moving the cursor with the keyboard instead of mouse clicking. The text gets added to the end in that case, as we're not tracking cursor changes via keystroke. It would be nice if we didn't HAVE to track the cursor via the events, but could just request it when we're inserting the variable.

Nice cleanup with the new components!

data-test-subj={`${paramsProperty}Input`}
value={inputTargetValue}
onChange={(e: React.ChangeEvent<HTMLInputElement>) => onChangeWithMessageVariable(e)}
onClick={(e: React.MouseEvent<HTMLInputElement, MouseEvent>) => onClickWithMessageVariable(e)}
Copy link
Member

Choose a reason for hiding this comment

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

for both the text field and text area, but not the JSON editor, onClick doesn't seem to be enough to get the cursor / insertion point, if you move the cursor with the keyboard instead of via mouse click. Eg, enter abc in a field, use the arrow key to move the cursor between the a and b, then insert a variable via mouse click. It gets placed after abc, not between a and b.

Is it possible to just get the selectionStart / selectionEnd fields only when needed (when inserting the variable), instead of trying to track it via events?

@YulNaumenko YulNaumenko requested a review from pmuellr July 2, 2020 18:31
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM - works as expected

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
triggers_actions_ui 104 +3 101

History

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

@YulNaumenko YulNaumenko merged commit 21efd23 into elastic:master Jul 3, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 3, 2020
…action text fields (elastic#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 3, 2020
* master: (32 commits)
  [Ingest Pipelines] Load from json (elastic#70297)
  [Rum Dashbaord] Rum selected service view (elastic#70579)
  [Uptime] Prevent duplicate requests on load for index status (elastic#70585)
  [ML] Changing shared module setup function parameters (elastic#70589)
  [Ingest Manager] Add ability to sort to agent configs and package configs (elastic#70676)
  [Alerting] document requirements for developing new action types (elastic#69164)
  Fixed adding an extra space character on selecting alert variable in action text fields (elastic#70028)
  [Maps] show vector tile labels on top (elastic#69444)
  chore(NA): upgrade to lodash@4 (elastic#69868)
  Add Snapshot Restore README with quick-testing steps. (elastic#70494)
  [EPM] Use higher priority than default templates (elastic#70640)
  [Maps] Fix cannot select Solid fill-color when removing fields (elastic#70621)
  [kbn/optimizer] only build specified themes (elastic#70389)
  Fix saved query modal overlay (elastic#68826)
  Update component templates list to render empty prompt inside of content container. Show detail panel when deep-linked, even if there are no component templates. (elastic#70633)
  [Security Solution] Renames the `Investigate in Resolver` Timeline action (elastic#70634)
  fix 400 error on initial signals search (elastic#70618)
  [Maps] fix unable to edit heatmap metric (elastic#70606)
  Update network idle timeout (elastic#70629)
  [APM] Disable flaky useFetcher test (elastic#70638)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 3, 2020
* master: (199 commits)
  [Telemetry] Add documentation about Application Usage (elastic#70624)
  [Ingest Manager] Improve agent unenrollment with unenroll action (elastic#70031)
  Handle timeouts on creating templates (elastic#70635)
  [Lens] Add ability to set colors for y-axis series (elastic#70311)
  [Uptime] Use elastic charts donut (elastic#70364)
  [Ingest Manager] Update registry URL to point to snapshot registry (elastic#70687)
  [Composable template] Create / Edit wizard (elastic#70220)
  [APM] Optimize services overview (elastic#69648)
  [Ingest Pipelines] Load from json (elastic#70297)
  [Rum Dashbaord] Rum selected service view (elastic#70579)
  [Uptime] Prevent duplicate requests on load for index status (elastic#70585)
  [ML] Changing shared module setup function parameters (elastic#70589)
  [Ingest Manager] Add ability to sort to agent configs and package configs (elastic#70676)
  [Alerting] document requirements for developing new action types (elastic#69164)
  Fixed adding an extra space character on selecting alert variable in action text fields (elastic#70028)
  [Maps] show vector tile labels on top (elastic#69444)
  chore(NA): upgrade to lodash@4 (elastic#69868)
  Add Snapshot Restore README with quick-testing steps. (elastic#70494)
  [EPM] Use higher priority than default templates (elastic#70640)
  [Maps] Fix cannot select Solid fill-color when removing fields (elastic#70621)
  ...
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 5, 2020
…action text fields (elastic#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 5, 2020
…action text fields (elastic#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 6, 2020
…action text fields (elastic#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
YulNaumenko added a commit that referenced this pull request Jul 6, 2020
…action text fields (#70028) (#70804)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 6, 2020
…action text fields (elastic#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index/es_index_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index/es_index_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty/pagerduty_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty/pagerduty_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/slack/slack_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/slack/slack_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook/webhook_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook/webhook_params.tsx
YulNaumenko added a commit that referenced this pull request Jul 6, 2020
…le in action text fields (#70028) (#70829)

* Fixed adding an extra space character on selecting alert variable in action text fields (#70028)

* Fixed adding an extra space character on selecting alert variable in action text fields.

* Made components for variables to be able to insert the variable by the cursor position

* cleanup

* Added variables support for all components

* update on handle selections for text

* Fixed functional tests
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/email/email_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index/es_index_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/es_index/es_index_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty/pagerduty_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/pagerduty/pagerduty_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/server_log/server_log_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/slack/slack_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/slack/slack_params.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook/webhook_params.test.tsx
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/webhook/webhook_params.tsx

* fixed failing jest test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Alerting release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.1 v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding an alert variable in PagerDuty action and selecting a value results in an extra space character
5 participants