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.

Sign up for GitHub

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

Jump to bottom

Clicking cancel in saved query save modal doesn't close it #62774

Merged
merged 6 commits into from
Apr 14, 2020
Merged

Clicking cancel in saved query save modal doesn't close it #62774

merged 6 commits into from
Apr 14, 2020

Conversation

VladLasitsa
Copy link
Contributor

Closes #62463

Summary

I investigated this issue and found out the following:

  • the problem appeared after these changes (applying validation by blur event on the input instead of input changes): Save query form validation on blur #43726
  • validation that is triggered on the blur event on the input (actually any click outside the input including click on the Cancel button) leads to the rerender of the modal window and showing the error message. This message increases the height of the modal window because of this the click doesn't fall on the "Cancel" button (button moves below).

I offer the next solutions:

  • added autotrim by blur event.
  • validation applied only by click on Save instead of by blur event
  • if the user doesn't type a name then the Save button is disabled. (the state is updating after any change in input)

save_query2

Checklist

Delete any items that are not applicable to this PR.

For maintainers

alexwizp
alexwizp reviewed Apr 7, 2020
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

Current behavior looks good to me, some comments were added

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa VladLasitsa marked this pull request as ready for review April 8, 2020 07:55
@VladLasitsa VladLasitsa requested a review from a team as a code owner April 8, 2020 07:55
@VladLasitsa VladLasitsa self-assigned this Apr 8, 2020
@VladLasitsa VladLasitsa added release_note:skip Skip the PR/issue when compiling release notes Team:AppArch v7.8.0 v8.0.0 labels Apr 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@VladLasitsa
Copy link
Contributor Author

@flash1293, Could you please review PR?

alexwizp
alexwizp approved these changes Apr 13, 2020
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you

lizozom
lizozom suggested changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Changes cause Kibana to get stuck -

  1. Save a "Saved Query"
  2. Open the saved query menu
  3. Try clearing the saved query
  4. Kibana gets completely stuck and won't release unless you kill the process.

src/plugins/data/public/ui/saved_query_form/save_query_form.tsx
{
defaultMessage: 'Name cannot contain leading or trailing whitespace',
defaultMessage: 'Save query text and filters that you want to use again.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Save search bar configuration for future use.

@VladLasitsa
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

lizozom
lizozom approved these changes Apr 14, 2020
View reviewed changes
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Bug happens to me on master as well.
So LGTM

@VladLasitsa VladLasitsa merged commit 4ce2412 into elastic:master Apr 14, 2020
@VladLasitsa VladLasitsa added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 14, 2020
@VladLasitsa VladLasitsa mentioned this pull request Apr 14, 2020
Merged
VladLasitsa added a commit to VladLasitsa/kibana that referenced this pull request Apr 14, 2020
@VladLasitsa @elasticmachine
Clicking cancel in saved query save modal doesn't close it (elastic#6…
d231333 
…2774)

* use validation by clicking on save, added autotrim

* Fixes form errors.

* Fixed comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
VladLasitsa added a commit that referenced this pull request Apr 15, 2020
@VladLasitsa @elasticmachine
Clicking cancel in saved query save modal doesn't close it (#62774) (#…
16c5771 
…63479)

* use validation by clicking on save, added autotrim

* Fixes form errors.

* Fixed comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 15, 2020
@gmmorris
Merge branch 'master' into alerting/deleted-alert-error
636ff99 
* master: (29 commits)
  Add test:jest_integration npm script (elastic#62938)
  [data.search.aggs] Remove service getters from agg types (AggConfig part) (elastic#62548)
  [Discover] Fix broken setting of bucketInterval (elastic#62939)
  Disable adding conditions when in alert management context. (elastic#63514)
  [Alerting] fixes to allow pre-configured actions to be executed (elastic#63432)
  adding useMemo (elastic#63504)
  [Maps] fix double fetch when filter pill is added (elastic#63024)
  [Lens] Fix missing formatting bug in "break down by" (elastic#63288)
  [SIEM] [Cases] Removed double pasted line (elastic#63507)
  [Reporting] Improve functional test steps (elastic#63259)
  [SIEM][CASE] Tests for server's configuration API (elastic#63099)
  [SIEM] [Cases] Case container unit tests (elastic#63376)
  [ML] Improving parsing of large uploaded files (elastic#62970)
  [ML] Listing global calendars on the job management page (elastic#63124)
  [Ingest][Endpoint] Add Ingest rest api response types for use in Endpoint (elastic#63373)
  Add help text to form fields (elastic#63165)
  [ML] Converts utils Mocha tests to Jest (elastic#63132)
  [Metrics UI] Refactor With* containers to hooks (elastic#59503)
  [NP] Migrate logstash server side code to NP (elastic#63135)
  Clicking cancel in saved query save modal doesn't close it (elastic#62774)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 15, 2020
@gmmorris
Merge branch 'alerting/alert-services-mock' into alerting/es-query
75fe882 
* alerting/alert-services-mock: (107 commits)
  removed unused import
  added alert services mock and use it in siem
  [Metrics UI] Refactor With* containers to hooks (elastic#59503)
  [NP] Migrate logstash server side code to NP (elastic#63135)
  Clicking cancel in saved query save modal doesn't close it (elastic#62774)
  [Lens] Migration from 7.7 (elastic#62879)
  [Lens] Fix bug where suggestions didn't use filters (elastic#63293)
  Task/linux events (elastic#63400)
  [Remote clusters] guard against usageCollection plugin if unav… (elastic#63284)
  [Uptime] Remove pings graphql (elastic#59392)
  Index Pattern Field class - factor out copy_field code for future typescripting (elastic#63083)
  [EPM] add/remove package in package settings page (elastic#63389)
  Adjust API authorization logging (elastic#63350)
  Revert FTR: add chromium-based Edge browser support (elastic#61684) (elastic#63448)
  [Event Log] Adds namespace into save objects (elastic#62974)
  document code splitting for client code (elastic#62593)
  Escape single quotes surrounded by double quotes (elastic#63229)
  [Endpoint] Update cli mapping to match endpoint package (elastic#63372)
  update in-app links to metricbeat configuration docs (elastic#63295)
  investigation notes field (documentation / metadata) (elastic#63386)
  ...
wayneseymour pushed a commit that referenced this pull request Apr 15, 2020
@VladLasitsa @elasticmachine @wayneseymour
Clicking cancel in saved query save modal doesn't close it (#62774)
7d9ad9e 
* use validation by clicking on save, added autotrim

* Fixes form errors.

* Fixed comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
tsullivan pushed a commit to tsullivan/kibana that referenced this pull request Apr 15, 2020
@VladLasitsa @elasticmachine @tsullivan
Clicking cancel in saved query save modal doesn't close it (elastic#6…
5ffa7b4 
…2774) (elastic#63479)

* use validation by clicking on save, added autotrim

* Fixes form errors.

* Fixed comments

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@rayafratkina rayafratkina added the Feature:Query Bar Querying and query bar features label Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizozom lizozom lizozom approved these changes

@alexwizp alexwizp alexwizp approved these changes

Assignees

@VladLasitsa VladLasitsa

Labels
Feature:Query Bar Querying and query bar features release_note:fix v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Clicking cancel in saved query save modal doesn't close it
6 participants
@VladLasitsa @elasticmachine @kibanamachine @lizozom @alexwizp @rayafratkina