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

Cleanup advanced settings #93437

Merged
merged 8 commits into from
Mar 17, 2021
Merged

Cleanup advanced settings #93437

merged 8 commits into from
Mar 17, 2021

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Mar 3, 2021 •
edited
Loading

Summary

Release Notes: The discover:aggs:terms:size advanced setting got removed since it was no longer used.

Minor cleanup of advanced settings:

  • Remove the discover:aggs:terms:size setting. It's no longer used since we switched using uiActions to visualiza a field.
  • Use the existing page reload feature for the "Enable rollup" advanced setting.

Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~- [ ] Documentation was added for features that require explanation or tutorials
- [ ] Unit or functional tests were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
- [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
- [ ] This was checked for cross-browser compatibility

For maintainers

@timroes timroes added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0 labels Mar 4, 2021
@timroes
Copy link
Contributor Author

timroes commented Mar 8, 2021

@elasticmachine merge upstream

@timroes timroes marked this pull request as ready for review March 9, 2021 10:33
@timroes timroes requested review from a team as code owners March 9, 2021 10:33
@timroes timroes requested a review from a team March 9, 2021 10:33
@timroes timroes requested a review from a team as a code owner March 9, 2021 10:33
@elasticmachine
Copy link
Contributor

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

Bamieh
Bamieh approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@Bamieh Bamieh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Telemetry changes LGTM. It seems you need to run node scripts/i18n_check --fix to get the translations fixed

kertal
kertal approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Kibana App owned Code LGTM 👍 , tested locally with MacOs, Safari. Visualize button in Discover unsurprisingly works like before. Change of rollup - advanced setting triggers a One or more settings require you to reload the page to take effect. - toast

wylieconlon
wylieconlon reviewed Mar 9, 2021
View reviewed changes
Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Do you need to add this to the 7.13 release notes in some way? Or maybe add it to whichever minor we removed the setting from?

@wylieconlon
Copy link
Contributor

Found another setting that we could remove: #94212

@timroes timroes added release_note:deprecation auto-backport Deprecated - use backport:version if exact versions are needed and removed release_note:skip Skip the PR/issue when compiling release notes labels Mar 11, 2021
@timroes
Copy link
Contributor Author

timroes commented Mar 11, 2021

@wylieconlon good call, but I'd prefer to have that in a separate PR, given that it's not just removing the setting.

yuliacech
yuliacech approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @timroes! ES UI rollup changes LGTM 👍

@timroes
Copy link
Contributor Author

timroes commented Mar 16, 2021

@elasticmachine merge upstream

@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
discover 402.0KB 401.9KB -57.0B
visualize 99.3KB 99.2KB -57.0B
total -114.0B

History

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

@timroes timroes merged commit 34f8f62 into elastic:master Mar 17, 2021
@timroes timroes deleted the cleanup-advanced-settings branch March 17, 2021 13:44
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Mar 17, 2021
@kibanamachine
Cleanup advanced settings (elastic#93437)
578f911 
* Cleanup advanced settings

* Fix translations/telemetry settings

* Fix telemetry settings

* Fix discover api docs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Mar 17, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.x / #94803

Successful backport PRs will be merged automatically after passing CI.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

3 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 24, 2021
kibanamachine added a commit that referenced this pull request Mar 24, 2021
@kibanamachine
Cleanup advanced settings (#93437) (#94803)
72ef242 
* Cleanup advanced settings

* Fix translations/telemetry settings

* Fix telemetry settings

* Fix discover api docs

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Tim Roes <tim.roes@elastic.co>
@kertal kertal mentioned this pull request Dec 21, 2021
Open
@lukasolson lukasolson mentioned this pull request Dec 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wylieconlon wylieconlon wylieconlon left review comments

@kertal kertal kertal approved these changes

@haywoood haywoood haywoood approved these changes

@Bamieh Bamieh Bamieh approved these changes

@yuliacech yuliacech yuliacech approved these changes

Assignees
No one assigned
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Discover Discover Application release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@timroes @elasticmachine @wylieconlon @kibanamachine @kertal @haywoood @Bamieh @yuliacech