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

[Upgrade Assistant] Fix filter deprecations search filter #57541

Merged
merged 9 commits into from
Feb 17, 2020

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 13, 2020

Summary

Fix #57054

Release Notes

Fixed an issue with filtering deprecations in Upgrade Assistant where invalid Regular Expressions would fail silently. The UI now reports the issue.

Screenshots

First iteration Screenshot 2020-02-13 at 10 11 35

Screenshot 2020-02-13 at 16 51 09

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@jloleysens jloleysens added release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Upgrade Assistant v7.7.0 v7.6.1 labels Feb 13, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@cjcenizal
Copy link
Contributor

@jloleysens Inserting the callout below the search box throws it out of alignment with the controls on the right, which looks a little weird. I wonder if there's a solution to this that doesn't alter the layout? Maybe a red alert icon to the right of the search box, with a tooltip that explains the error in more detail?

@jloleysens
Copy link
Contributor Author

@cjcenizal I think to keep in line with something like Index Management

Screenshot 2020-02-13 at 16 30 18

We can just move the callout rendering location to below the controls?

@cjcenizal
Copy link
Contributor

Good idea!

The previous callout layout looked off-center next to the controls in the table.
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this and adding tests!

I left one nit about the text. I also had a couple comments around the code, but I see they relate to code that already existed, so feel free to ignore.

<EuiFlexItem grow={true}>
<EuiFieldSearch
isInvalid={filterInvalid}
aria-label="Filter"
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n

<EuiFieldSearch
isInvalid={filterInvalid}
aria-label="Filter"
placeholder={intl.formatMessage({
Copy link
Contributor

Choose a reason for hiding this comment

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

can you switch to i18n.translate() here instead?

<EuiFlexGroup direction="column" responsive={false}>
<EuiFlexItem grow={true}>
<EuiFlexGroup alignItems="center" wrap={true} responsive={false}>
<EuiFlexItem grow={true}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe true is the default

title={i18n.translate(
'xpack.upgradeAssistant.checkupTab.controls.filterErrorMessageLabel',
{
defaultMessage: 'Filter Invalid: {searchTermError}',
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should probably be sentence case per https://elastic.github.io/eui/#/guidelines/writing.

Update "Filter Invalid:" to sentence case
Remove inject intl wrapper from CheckupControls component
Remove unnecessary grow={true}
@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
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

@jloleysens jloleysens merged commit 26fdc4a into elastic:master Feb 17, 2020
@jloleysens jloleysens deleted the fix/ua/search-filter branch February 17, 2020 10:04
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 17, 2020
)

* Made eui search field not a controlled component
Added validateRegExpString util

* Update error message display. Use EuiCallOut and i18n to replicate other search filter behaviour, e.g. index management.

* Remove unused variable

* Update Jest snapshot

* Updated layout for callout

The previous callout layout looked off-center next to the controls in the table.

* Update copy and remove intl

Update "Filter Invalid:" to sentence case
Remove inject intl wrapper from CheckupControls component
Remove unnecessary grow={true}

* Updated Jest component snapshot

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 17, 2020
)

* Made eui search field not a controlled component
Added validateRegExpString util

* Update error message display. Use EuiCallOut and i18n to replicate other search filter behaviour, e.g. index management.

* Remove unused variable

* Update Jest snapshot

* Updated layout for callout

The previous callout layout looked off-center next to the controls in the table.

* Update copy and remove intl

Update "Filter Invalid:" to sentence case
Remove inject intl wrapper from CheckupControls component
Remove unnecessary grow={true}

* Updated Jest component snapshot

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 17, 2020
…re/files-and-filetree

* 'master' of github.com:elastic/kibana: (139 commits)
  Move Ace XJSON lexer-rules, worker and utils to es_ui_shared (elastic#57563)
  [Upgrade Assistant] Fix filter deprecations search filter (elastic#57541)
  [ML] New Platform server shim: update indices routes (elastic#57685)
  Bump redux dependencies (elastic#53348)
  [Index management] Client-side NP ready (elastic#57295)
  change id of x-pack event_log plugin to eventLog (elastic#57612)
  [eventLog] get kibana.index name from config instead of hard-coding it (elastic#57607)
  revert
  allow using any path to generate
  fixes ui titles (elastic#57535)
  Fix login redirect for expired sessions (elastic#57157)
  Expose Vis on the contract as it requires visTypes (elastic#56968)
  [SIEM][Detection Engine] Fixes queries to ignore errors when signals index is not present
  [Remote clusters] Migrate client-side code out of legacy (elastic#57365)
  Fix failed test reporter for SIEM Cypress use (elastic#57240)
  skip flaky suite (elastic#45244)
  update chromedriver to 80.0.1 (elastic#57602)
  change slack action to only report on whitelisted host name (elastic#57582)
  [kbn/optimizer] throw errors into stream on invalid completion (elastic#57735)
  moving visualize/utils to new platform (elastic#56650)
  ...
jloleysens added a commit that referenced this pull request Feb 17, 2020
…57779)

* Made eui search field not a controlled component
Added validateRegExpString util

* Update error message display. Use EuiCallOut and i18n to replicate other search filter behaviour, e.g. index management.

* Remove unused variable

* Update Jest snapshot

* Updated layout for callout

The previous callout layout looked off-center next to the controls in the table.

* Update copy and remove intl

Update "Filter Invalid:" to sentence case
Remove inject intl wrapper from CheckupControls component
Remove unnecessary grow={true}

* Updated Jest component snapshot

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
jloleysens added a commit that referenced this pull request Feb 17, 2020
…57780)

* Made eui search field not a controlled component
Added validateRegExpString util

* Update error message display. Use EuiCallOut and i18n to replicate other search filter behaviour, e.g. index management.

* Remove unused variable

* Update Jest snapshot

* Updated layout for callout

The previous callout layout looked off-center next to the controls in the table.

* Update copy and remove intl

Update "Filter Invalid:" to sentence case
Remove inject intl wrapper from CheckupControls component
Remove unnecessary grow={true}

* Updated Jest component snapshot

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Upgrade Assistant release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.1 v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UA] Putting * or ? in the beginning of search string causes all results to be shown in clusters or indices
5 participants