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

Rename kbn_vislib_vis_types plugin #53536

Merged
merged 29 commits into from
Jan 15, 2020
Merged

Rename kbn_vislib_vis_types plugin #53536

merged 29 commits into from
Jan 15, 2020

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Dec 18, 2019 •
edited
Loading

Release Note

Renames kbn_vislib_vis_types plugin to vis_type_vislib

@nickofthyme nickofthyme added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.6.0 labels Dec 18, 2019
@kibanamachine
Copy link
Contributor

💔 Build Failed

History

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

@elastic elastic deleted a comment from elasticmachine Dec 27, 2019
kindsun
kindsun approved these changes Jan 6, 2020
View reviewed changes
Copy link
Contributor

@kindsun kindsun left a comment

Choose a reason for hiding this comment

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

Maps lgtm!

  • code review
  • tested Maps app locally

@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jan 7, 2020
@elasticmachine
Copy link
Contributor

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

flash1293
flash1293 approved these changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Looks good to me overall. I didn't see any breakages and the things migrated look good to me. The rest of the stuff currently located in ui/vis can be moved as a second step.

I would like to rename this plugin to vis_type_vislib - @timroes is there a specific reason this is called differently than the others that's still valid or is this a leftover?

src/legacy/core_plugins/kbn_vislib_vis_types/public/plugin.ts Outdated
core: KbnVislibVisTypesCoreSetup,
{ expressions, visualizations, __LEGACY }: KbnVislibVisTypesPluginSetupDependencies
) {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ignore necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, thanks

src/legacy/core_plugins/kbn_vislib_vis_types/public/utils/collections.ts
@@ -81,10 +84,11 @@ const getChartTypes = () => [
},
];

export enum ChartModes {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's wrong with an enum here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer, nothing really.

In elastic-charts we only use object enums because of the bloat the standard ts enums create. elastic/elastic-charts#266

The regular enum would produce something like

enum Pet1 {
  Cat = 'Cat',
  Dog = 'Dog',
}

// transpiles to this
var Pet1;
(function (Pet1) {
    Pet1["Cat"] = "Cat";
    Pet1["Dog"] = "Dog";
})(Pet1 || (Pet1 = {}));

where a much cleaner output is

const Pet2 = Object.freeze({
  Cat: 'Cat' as 'Cat',
  Dog: 'Dog' as 'Dog',
})
type Pet2 = typeof Pet2.Cat | typeof Pet2.Dog;

// transpiles to this
const Pet2 = Object.freeze({
    Cat: 'Cat',
    Dog: 'Dog',
});

Using a const enum would be clearer but they are not yet supported in babel (babel/babel#8741)

cchaos
cchaos suggested changes Jan 7, 2020
View reviewed changes
.sass-lint.yml Outdated Show resolved Hide resolved
@nickofthyme nickofthyme added v7.7.0 and removed v7.6.0 labels Jan 9, 2020
nickofthyme and others added 3 commits January 9, 2020 10:06
@nickofthyme @cchaos
Update .sass-lint.yml
a0a721e 
Add `kbn_vislib_vis_types` to sass lint

Co-Authored-By: Caroline Horn <[email protected]>
@nickofthyme
address pr comments
e03d81c
@nickofthyme
Rename vis to vis_type_vislib
d54759b
cchaos
cchaos approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos 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 those 👍

@nickofthyme
Copy link
Contributor Author

@elasticmachine merge upstream

@nickofthyme
Copy link
Contributor Author

Waiting to put in v7.7.0 as no features are added.

@nickofthyme nickofthyme changed the title Shim of kbn_vislib_vis_types Shim vis_type_vislib Jan 13, 2020
@nickofthyme nickofthyme merged commit bd7d5eb into elastic:master Jan 15, 2020
@nickofthyme nickofthyme deleted the kpm/kbn_vislib_vis_types branch January 16, 2020 01:50
@nickofthyme nickofthyme mentioned this pull request Jan 16, 2020
Merged
nickofthyme added a commit to nickofthyme/kibana that referenced this pull request Jan 16, 2020
@nickofthyme
Shim vis_type_vislib (elastic#53536)
329b29d 
* shim of kbn_vislib_vis_types (now vis_type_vislib)
* Move vislib into vis_type_vislib plugin
* Convert remaining plugin files to typescript
* Rename vis to vis_type_vislib
nickofthyme added a commit that referenced this pull request Jan 16, 2020
@nickofthyme
Shim vis_type_vislib (#53536) (#55001)
2df5eb1 
* shim of kbn_vislib_vis_types (now vis_type_vislib)
* Move vislib into vis_type_vislib plugin
* Convert remaining plugin files to typescript
* Rename vis to vis_type_vislib
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
@nickofthyme @jkelastic
Shim vis_type_vislib (elastic#53536)
7e225c6 
* shim of kbn_vislib_vis_types (now vis_type_vislib)
* Move vislib into vis_type_vislib plugin
* Convert remaining plugin files to typescript
* Rename vis to vis_type_vislib
@nickofthyme nickofthyme added release_note:breaking and removed release_note:skip Skip the PR/issue when compiling release notes labels Jan 29, 2020
@nickofthyme nickofthyme changed the title Shim vis_type_vislib Renames kbn_vislib_vis_types plugin Jan 29, 2020
@nickofthyme nickofthyme changed the title Renames kbn_vislib_vis_types plugin Rename kbn_vislib_vis_types plugin Jan 29, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@nickofthyme nickofthyme mentioned this pull request May 17, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@mistic mistic mistic approved these changes

@kindsun kindsun kindsun approved these changes

@flash1293 flash1293 flash1293 approved these changes

@cchaos cchaos cchaos approved these changes

Assignees
No one assigned
Labels
Feature:NP Migration release_note:breaking Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@nickofthyme @kibanamachine @elasticmachine @cchaos @flash1293 @kindsun @mistic @peteharverson