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

[NP] Migrate Markdown to NP #62940

Merged
merged 8 commits into from
Apr 16, 2020
Merged

[NP] Migrate Markdown to NP #62940

merged 8 commits into from
Apr 16, 2020

Conversation

maryia-lapata
Copy link
Contributor

@maryia-lapata maryia-lapata commented Apr 8, 2020 •
edited
Loading

Summary

Part of #60097.

This PR moves the Markdown Visualization to kibana/new platform.

@maryia-lapata maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.8.0 labels Apr 8, 2020
@elasticmachine
Copy link
Contributor

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

@flash1293 flash1293 mentioned this pull request Apr 9, 2020
Closed
69 tasks
maryia-lapata
maryia-lapata commented Apr 9, 2020
View reviewed changes
src/plugins/vis_type_markdown/kibana.json Outdated
@@ -0,0 +1,6 @@
{
"id": "markdown_vis",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flash1293 I wonder if we change ids to camel case during the migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should do this and to make everything consistent, we should also rename the plugin to the same naming scheme we use with the folders: visTypeMarkdown.

Also the same things as in the metric vis type PR apply: #63096 (review)

@maryia-lapata
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@maryia-lapata maryia-lapata marked this pull request as ready for review April 12, 2020 08:40
@maryia-lapata maryia-lapata requested a review from a team as a code owner April 12, 2020 08:40
@flash1293
Copy link
Contributor

@elasticmachine merge upstream

flash1293
flash1293 requested changes Apr 14, 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.

The plugin id should be changed and there should be a enabled config key to disable the plugin. See #63096 (review)

src/plugins/vis_type_markdown/kibana.json Outdated
@@ -0,0 +1,6 @@
{
"id": "markdown_vis",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should do this and to make everything consistent, we should also rename the plugin to the same naming scheme we use with the folders: visTypeMarkdown.

Also the same things as in the metric vis type PR apply: #63096 (review)

cchaos
cchaos approved these changes Apr 14, 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.

👍 SASS file moves LGTM

flash1293
flash1293 approved these changes Apr 16, 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.

Tested and works as expected, LGTM 👍

@maryia-lapata
Merge remote-tracking branch 'remotes/kibana/master' into np-markdown
f140b59 
# Conflicts:
#	.github/CODEOWNERS
#	.i18nrc.json
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@maryia-lapata maryia-lapata merged commit bae2bda into elastic:master Apr 16, 2020
@maryia-lapata maryia-lapata deleted the np-markdown branch April 16, 2020 15:32
@maryia-lapata maryia-lapata mentioned this pull request Apr 17, 2020
Merged
maryia-lapata added a commit that referenced this pull request Apr 17, 2020
@maryia-lapata @elasticmachine
[7.x] [NP] Migrate Markdown to NP (#62940) (#63817)
7a50672 
* [NP] Migrate Markdown to NP (#62940)

* Migrate Markdown to NP

* Refactoring

* Add ConfigSchema, server, rename markdown_vis to visTypeMarkdown

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Remove codeowners

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@flash1293 flash1293 flash1293 approved these changes

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@maryia-lapata @elasticmachine @flash1293 @kibanamachine @cchaos