Skip to content

Rename chartLibrary setting to legacyChartsLibrary #86529

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
Merged
merged 3 commits into from
Dec 19, 2020
Merged

Rename chartLibrary setting to legacyChartsLibrary #86529

merged 3 commits into from
Dec 19, 2020

Conversation

@nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 18, 2020

Summary

This PR renames the current advance setting visualization:visualize:chartsLibrary to visualization:visualize:legacyChartsLibrary. This change reversed the previous behavior and is now enabled by default.

Additional cleanup of copy material and testing comments to reflect change.

A follow-up PR will disable this setting in master

Checklist

@nickofthyme nickofthyme added Feature:XYAxis XY-Axis charts (bar, area, line) v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Dec 18, 2020
@nickofthyme nickofthyme requested review from a team, gchaps, markov00 and timroes December 18, 2020 20:17
stratoula
stratoula reviewed Dec 18, 2020
View reviewed changes
src/plugins/vis_type_xy/server/plugin.ts Outdated
defaultMessage: 'Charts library',
[LEGACY_CHARTS_LIBRARY]: {
name: i18n.translate('visTypeXy.advancedSettings.visualization.legacyChartsLibrary.name', {
defaultMessage: 'Legact charts library',
Copy link
Contributor

@stratoula stratoula Dec 18, 2020

Choose a reason for hiding this comment

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

A typo here 🙂

Copy link
Contributor Author

@nickofthyme nickofthyme Dec 18, 2020

Choose a reason for hiding this comment

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

6d12b4d 😫

@stratoula
Copy link
Contributor

stratoula commented Dec 18, 2020

@nickofthyme something goes wrong. I have the switch to on but I see the new implementation. It seems that it works the opposite that it says.

@nickofthyme nickofthyme mentioned this pull request Dec 18, 2020
Merged
@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 18, 2020 •
edited
Loading

@nickofthyme something goes wrong. I have the switch to on but I see the new implementation. It seems that it works the opposite that it says.

Yeah sorry for the confusion, I should have done this before merging the main pr. This is just to rename the setting so that I can also backport this pr to 7.x.

Then once this is merged I will enable the setting to show the new implementation. See #86538

Update: I see, the setting doesn't trigger the new chart library when disabled...

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 18, 2020

Ugh forgot to flip the check in the plugin setup... fixed here ff8b456

stratoula
stratoula approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

And now it works 😄 Code LGTM. Tested in chrome and the setting works fine!

@nickofthyme
Copy link
Contributor Author

nickofthyme commented Dec 18, 2020

Thanks for that, I was scared the functional tests were all starting to fail 😱

@stratoula
Copy link
Contributor

stratoula commented Dec 18, 2020

I am pretty sure that now they will pass 😆

gchaps
gchaps approved these changes Dec 18, 2020
View reviewed changes
Copy link
Contributor

@gchaps gchaps left a comment

Choose a reason for hiding this comment

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

Text LGTM

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 18, 2020

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47298 48058 +760

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
visTypeVislib 46.6KB 46.7KB +13.0B
visTypeXy 67.6KB 67.6KB -55.0B
total -42.0B

History

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

@nickofthyme nickofthyme merged commit 7e9177d into elastic:master Dec 19, 2020
@nickofthyme nickofthyme deleted the rename-chart-setting branch December 19, 2020 01:07
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 21, 2020
@gmmorris
Merge branch 'master' into alerts/remove-generics-defaults-redux
561ebb1 
* master: (48 commits)
  Fix request with disabled aggregation (elastic#85696)
  [Security Solution][Detections][Threshold Rules] Threshold Rule Bug Fixes (elastic#84918)
  Removed a possibility to define two different names for Alert types on API and UI level. (elastic#86236)
  Bump Node.js from version 14.15.2 to 14.15.3 (elastic#86593)
  [index patterns] Fleep app - Keep saved object field list until field caps provides fields (elastic#85370)
  [Security Solutions] fix timeline tabs + layout (elastic#86581)
  Upgrade to hapi version 20 (elastic#85406)
  App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (elastic#82791)
  Rename chartLibrary setting to legacyChartsLibrary (elastic#86529)
  [CI] TeamCity updates (elastic#85843)
  [Maps] Use Json for mvt-tests (elastic#86492)
  [Rollup Jobs] Added autofocus to cron editor (elastic#86324)
  [Monitoring][Alerting] CCR read exceptions alert (elastic#85908)
  [CI] Bump memory for main CI workers (elastic#86541)
  Explicitly set Elasticsearch heap size during CI and local development (elastic#86513)
  [App Search] Updates to results on the documents view (elastic#86181)
  [Discover] Change default sort handling  (elastic#85561)
  [App Search] Convert DocumentCreationModal to DocumentCreationFlyout (elastic#86508)
  [App Search] Sample Engines should have access to the Crawler (elastic#86502)
  Fixed duplication of create new modal (elastic#86489)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 22, 2020
@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 22, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

3 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 23, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 24, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 25, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 28, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

6 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 29, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 30, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Dec 31, 2020

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Jan 1, 2021

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Jan 4, 2021

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

kibanamachine commented Jan 5, 2021

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 86529 or prevent reminders by adding the backport:skip label.

@stratoula
Copy link
Contributor

stratoula commented Jan 5, 2021

@nickofthyme this needs backport right?

@nickofthyme nickofthyme mentioned this pull request Jan 5, 2021
Merged
@nickofthyme
Copy link
Contributor Author

nickofthyme commented Jan 5, 2021

Yes, thanks for the reminder.

@kibanamachine
Copy link
Contributor

kibanamachine commented Jan 6, 2021

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 Jan 6, 2021
stratoula pushed a commit that referenced this pull request Jan 6, 2021
@nickofthyme
Rename chartLibrary setting to legacyChartsLibrary (#86529) (#87328)
853dfae 
* rename chartLibrary setting to legacyChartsLibrary

* fix spelling

* fix plugin setting check boolean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stratoula stratoula stratoula approved these changes

@timroes timroes Awaiting requested review from timroes

@markov00 markov00 Awaiting requested review from markov00

+1 more reviewer

@gchaps gchaps gchaps approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:XYAxis XY-Axis charts (bar, area, line) release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nickofthyme @stratoula @kibanamachine @gchaps