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

[Advanced settings] Reset to default for empty strings #85137

Merged
merged 2 commits into from
Dec 10, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Dec 7, 2020

Summary

Fixes #84876.

I can see that we check if the value is empty in order to not display the Reset to default button link. I agree with Marco that this is a bug rather than functionality that we want.

With removing this check the reset to default is displayed now when the user empties an advanced setting.

Screenshot 2020-12-07 at 4 10 14 PM

I can't think of any scenario that we don't want this. I checked on fields that they are empty by default and this change doesn't affect the way that they behave as the empty string is the default value and the is_default_value.ts works as expected. For example for the Index pattern placeholder setting.

Screenshot 2020-12-07 at 6 20 14 PM

Checklist

Delete any items that are not applicable to this PR.

@stratoula stratoula added Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:fix v7.11.0 v8.0.0 labels Dec 7, 2020
@stratoula stratoula marked this pull request as ready for review December 8, 2020 11:10
@stratoula stratoula requested a review from a team December 8, 2020 11:10
@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Dec 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
advancedSettings 919.6KB 919.6KB -20.0B

Distributable file count

id before after diff
default 46981 47741 +760

History

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

Copy link
Contributor

@mbondyra mbondyra left a comment

Choose a reason for hiding this comment

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

tested on FF, code LGTM 🆗

@stratoula stratoula merged commit b46655d into elastic:master Dec 10, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request Dec 10, 2020
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
stratoula added a commit that referenced this pull request Dec 10, 2020
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 10, 2020
* master: (53 commits)
  Fixing recovered instance reference bug (elastic#85412)
  Switch to new elasticsearch client for Visualizations (elastic#85245)
  Switch to new elasticsearch client for TSVB (elastic#85275)
  Switch to new elasticsearch client for Vega (elastic#85280)
  [ILM] Add shrink field to hot phase (elastic#84087)
  Add rolling-file appender to core logging (elastic#84735)
  [APM] Service overview: Dependencies table (elastic#83416)
  [Uptime ]Update empty message for certs list (elastic#78575)
  [Graph] Fix graph saved object references (elastic#85295)
  [APM] Create new API's to return Latency and Throughput charts (elastic#85242)
  [Advanced settings] Reset to default for empty strings (elastic#85137)
  [SECURITY SOLUTION] Bundles _source -> Fields + able to sort on multiple fields in Timeline (elastic#83761)
  [Fleet] Update agent listing for better status reporting (elastic#84798)
  [APM] enable 'sanitize_field_names' for Go (elastic#85373)
  Update dependency @elastic/charts to v24.4.0 (elastic#85452)
  Introduce external url service (elastic#81234)
  Deprecate disabling the security plugin (elastic#85159)
  [FLEET] New Integration Policy Details page for use in Integrations section (elastic#85355)
  [Security Solutions][Detection Engine] Fixes one liner access control with find_rules REST API
  chore: 🤖 remove extraPublicDirs (elastic#85454)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Kibana Management Feature label for Data Views, Advanced Setting, Saved Object management pages release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Advanced Settings] Reset to default button doesn't appear with empty strings
5 participants