Charts plugin (combining ui/color_maps and EuiUtils) by nickofthyme · Pull Request #55469 · elastic/kibana

nickofthyme · 2020-01-21T21:07:34Z

Summary

The EuiUtils plugin is now the theme service of the new Charts plugin
ui/color_maps is now the colorMaps service of the new Charts plugin
Fix all imports of each to pull from new Charts plugin
Add theme methods to both setup and start contract of Charts plugin
Move ui/vis/components/colors into new Charts plugin

Checklist

This was checked for cross-browser compatibility, including a check against IE11
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Add `kbn_vislib_vis_types` to sass lint Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

nreese

maps changes LGTM
code review, tested in chrome

flash1293 · 2020-01-24T09:07:00Z

src/plugins/charts/public/plugin.ts

+    this.colorsService.init(uiSettings);
+
+    return {
+      colorMaps: colorMapsService,


You are exposing the color maps service via the contract, but it looks like it is just offering static helper functions and is not tied to any internal state of the plugin - is this correct? If yes you can just re-export it from the top level index.ts of the charts plugin and import where needed. Then this problem is also solved directly.

flash1293 · 2020-01-24T09:45:01Z

src/plugins/charts/public/services/colors/colors.ts

+
+    return (value: string) => {
+      // @ts-ignore
+      return colorMapping[value] || this.mappedColors.get(value);


If you specify colorMapping as Partial<Record<string, string>> in https://github.com/elastic/kibana/pull/55469/files#diff-4026f17dfd1c005f7bc2f012cb664948R50 , then you don't need to overwrite type checking here.

flash1293 · 2020-01-24T09:46:34Z

src/plugins/charts/public/services/colors/colors.ts

+    this._mappedColors = new MappedColors(uiSettings);
+  }
+
+  vislibColor(arrayOfStringsOrNumbers?: any, colorMapping = {}) {


I know it's just used by vislib right now, but can we rename that function? Maybe createColorLookupFunction - naming is hard :D But I think it's better than having that legacy name in here

jloleysens

Tested locally in watcher and EUI utils looks to be working as expected.

Left some non-blocker comments.

jloleysens · 2020-01-24T10:53:26Z

x-pack/plugins/watcher/__jest__/client_integration/helpers/app_context.mock.tsx

  // For our test harness, we don't use this mocked out http service
  http: httpServiceMock.createSetupContract(),
-};
+} as any;


Could we rather scope the as any here to theme: {...} as any?

Yup great point

jloleysens · 2020-01-24T11:16:14Z

src/plugins/charts/public/plugin.ts

+}
+
+/** @public */
+export type ChartsPluginStart = ChartsPluginSetup;


Is it necessary to expose the same interface for start and setup from this plugin? This seems like something that would most naturally fit into just setup.

Also, if we just expose at setup time we can remove the need to check for inited services and remove init step entirely. Not a blocker though.

I don't think it's necessary long term but it was currently being used in both so I resorted to the init approach over using getStartServices.

nickofthyme · 2020-01-24T23:49:35Z

@nreese Could you check this once more please? I moved some services to static exports.

flash1293

LGTM, tested and still seems to work fine for me

nreese

lgtm

kibanamachine · 2020-01-27T23:50:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8c1da89

History

💔 Build #22717 failed 52344b6
💚 Build #22491 succeeded de77fef
💔 Build #22438 failed be4fb9f
💔 Build #22431 failed 9449adb
💔 Build #22263 failed 6cca596

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

* Combine ui/color_maps and EuiUtils into new Charts plugin * EuiUtils is now the theme service * ui/color_maps is now the colorMaps service * Fix all imports of each to pull from new Charts plugin * Add theme methods to both setup and start contracts * Move and jestify heatMapColors tests * Convert remaining js files to ts * Move vis/color to Charts plugin * Update missed visTypeVislib naming

* master: (21 commits) [SIEM][Detection Engine] critical blocker updates to latest ECS version [Monitoring] Fix inaccuracies in logstash pipeline listing metrics (elastic#55868) Resetting errors and removing duplicates (elastic#56054) Add flag to opt out from sub url tracking (elastic#55672) [SIEM][Detection Engine] critical bug, fixes duplicate tags [ML] Anomaly Detection: Fix persist/restore of refreshInterval in globalState. (elastic#56113) [ML] Single Metric Viewer: Fix annnotations refresh. (elastic#56107) adapt ObjectToConfigAdapter.getFlattenedPaths to consider arrays as final values (elastic#56105) Add Appender.receiveAllLevels option to fix LegacyAppender (elastic#55752) [ML] Process delimited files like semi-structured text (elastic#56038) Charts plugin (combining ui/color_maps and EuiUtils) (elastic#55469) fix tutorial documentation (elastic#55996) [ML] Fix persist/restore of time/refreshInterval in data visualizer. (elastic#56122) [Index Management] Fix errors with validation (elastic#56072) [Index Management] Add try/catch when parsing index filter from URI (elastic#56051) [NP] add HTTP resources testing strategies (elastic#54908) [ML] Single Metric Viewer: Fix brush update on short recent timespans. (elastic#56125) [Uptime] Add timeout for slow process to skipped functional tests (elastic#56065) refactor (elastic#56121) Move tests in dashboard into appropriate folders (elastic#55304) ...

nickofthyme and others added 30 commits December 18, 2019 14:03

Initial shim of kbn_vislib_vis_types

7b892cc

Move vislib into kbn_vislib_vis_types plugin

ab037a5

Fix import errors in test files

166665f

Fix dashboard style issue

de9ef60

Fix Internationalization check

8775808

Fix jest test

73713fa

Merge branch 'master' into kpm/kbn_vislib_vis_types

26d6fc0

Convert remaining plugin files to typescript

17c667b

Fix mappedColors import

e21c52e

Move ui/ and plugins/ services and imports to legacy file

e11a761

Remove none type changes

b416b5e

Merge branch 'master' into kpm/kbn_vislib_vis_types

73195f9

Fix jest mock imports

216365e

remove unnecessary persisted_state imports

607242c

Remove angular dependency from vislib tooltip and move into __Legacy

dc1668d

Merge branch 'master' into kpm/kbn_vislib_vis_types

d58a943

Fix linting errors

f94e179

Fix linting errors

2d383d3

Update jest snapshots

ba6eea9

Migrate remaining stateful deps to top-level

ec8b095

Fix broken vislib mocha tests after removing Private instance

e918df2

Merge branch 'master' into kpm/kbn_vislib_vis_types

48f4958

Merge branch 'master' into kpm/kbn_vislib_vis_types

e605747

Update .sass-lint.yml

a0a721e

Add `kbn_vislib_vis_types` to sass lint Co-Authored-By: Caroline Horn <549577+cchaos@users.noreply.github.com>

address pr comments

e03d81c

Rename vis to vis_type_vislib

d54759b

Merge branch 'master' into kpm/kbn_vislib_vis_types

790c9bc

Merge branch 'master' into kpm/kbn_vislib_vis_types

0f7dca3

Remove angular dependencey from vis/tooltip

cc508cc

Move tooltip logic into vislib

b6f3678

Fix jest test and add charts plugin mock

3163a45

nickofthyme marked this pull request as ready for review January 23, 2020 00:49

nickofthyme requested a review from a team as a code owner January 23, 2020 00:49

nreese approved these changes Jan 23, 2020

View reviewed changes

nickofthyme added 3 commits January 23, 2020 12:03

Merge branch 'master' into kpm/charts-plugin

6b00cea

Move vis/color to Charts plugin

efccb73

Fix consumers and tests for Charts#color service

6cca596

nickofthyme requested a review from jloleysens January 23, 2020 22:58

flash1293 reviewed Jan 24, 2020

View reviewed changes

jloleysens approved these changes Jan 24, 2020

View reviewed changes

nickofthyme added 5 commits January 24, 2020 10:47

Update missed visTypeVislib naming

b6378b7

Move static charts methods to top-level exports

7159711

Merge branch 'master' into kpm/charts-plugin

9449adb

Fix remaining pr comments

be4fb9f

Fix type check errors

de77fef

nickofthyme requested review from flash1293 and nreese January 24, 2020 23:48

flash1293 approved these changes Jan 27, 2020

View reviewed changes

Merge branch 'master' into kpm/charts-plugin

52344b6

nreese approved these changes Jan 27, 2020

View reviewed changes

Merge branch 'master' into kpm/charts-plugin

8c1da89

nickofthyme merged commit 05ed2d6 into elastic:master Jan 28, 2020

nickofthyme deleted the kpm/charts-plugin branch January 28, 2020 14:14

nickofthyme mentioned this pull request Jan 28, 2020

[7.x] Charts plugin (combining ui/color_maps and EuiUtils) (#55469) #56151

Merged

Conversation

nickofthyme commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickofthyme commented Jan 24, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

nreese left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 27, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nickofthyme commented Jan 21, 2020 •

edited

Loading