Markdown vis type np shim #38767
Markdown vis type np shim #38767
Conversation
💔 Build Failed |
ppisljar
force-pushed
the
markdownVisTypeNPShim
branch
from
a703dc6 to
11c5da3
Compare
💔 Build Failed |
ppisljar
added
:AppArch
non-issue
|
Pinging @elastic/kibana-app-arch |
ppisljar
force-pushed
the
markdownVisTypeNPShim
branch
from
11c5da3 to
60e9b37
Compare
💔 Build Failed |
ppisljar
force-pushed
the
markdownVisTypeNPShim
branch
from
60e9b37 to
dbca727
Compare
💔 Build Failed |
ppisljar
force-pushed
the
markdownVisTypeNPShim
branch
from
dbca727 to
1b377f3
Compare
💔 Build Failed |
💚 Build Succeeded |
| } | ||
|
|
||
| }); | ||
| visTypes: ['plugins/markdown_vis_type/index'], |
There was a problem hiding this comment.
i tried with public dir but that doesn't seem to work ? (what data plugin does for example) seems in that case we need to have something consuming the plugin.
There was a problem hiding this comment.
I asked few people last week what we should use to load shims, and create a new uiExport in case we want to use that.
@joshdover suggested to simply use hacks uiExport.
@elastic/kibana-platform maybe it is worth putting into migration guide the suggested way to load client-side shims?
i tried with public dir but that doesn't seem to work ?
I don't know what publicDir does, but my guess always was that it tells Webpack where is the "./public" folder of the plugin.
| visualizationsSetup.types.VisTypesRegistryProvider.register(() => markdownVis); | ||
| functionsRegistry.register(kibanaMarkdown); | ||
| } | ||
|
|
There was a problem hiding this comment.
It looks like core's plugin_service calls all setup(), start() and stop() methods without checking if they exist, thus we need to provide start() here, too.
This is also one reason why it is useful to create real NP plugins; as only then we can really know if they work.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "name": "markdown_vis_type", | |||
There was a problem hiding this comment.
If we named "vis-type" plugins as vis_type_markdown instead, they would all be in one place in folder when sorted alphabetically. What do you think?
| @@ -0,0 +1,54 @@ | |||
| /* | |||
There was a problem hiding this comment.
Maybe while you are at it, also .js 👉 .ts?
| "markdownVis.markdownDescription": "マークダウン構文でドキュメントを作成します", | ||
| "markdownVis.params.fontSizeLabel": "フォントサイズ ({fontSize} pt)", | ||
| "markdownVis.params.helpLinkLabel": "ヘルプ", | ||
| "markdownVis.params.openLinksLabel": "新規タブでリンクを開く", |
There was a problem hiding this comment.
All these translations: should they be removed? or renamed? E.g:
"markdownVisType.params.openLinksLabel": "新規タブでリンクを開く",
There was a problem hiding this comment.
removed, then our translation team will add them back, the tool they use will find this as renamed.
ppisljar
force-pushed
the
markdownVisTypeNPShim
branch
from
cbe1649 to
ab27541
Compare
💚 Build Succeeded |
💚 Build Succeeded |
| public stop() {} | ||
| } | ||
|
|
||
| new VisTypeMarkdownPlugin().setup(visualizations); |
There was a problem hiding this comment.
I guess visualizations should be just one of the plugin contracts, not the whole object, also you may want to inject the data plugin contract.
import { functionsRegistry } from '../../interpreter/public/registries';
import { visualizations, VisualizationsSetup } from '../../visualizations/public';
export interface SetupDeps {
visualizations: VisualizationsSetup;
data: any;
}
class VisTypeMarkdownPlugin {
constructor() {}
public setup({visualizations, data}: SetupDeps) {
visualizations.types.VisTypesRegistryProvider.register(() => markdownVis);
data.expressions.registerFunction(kibanaMarkdown);
}
public start() {}
public stop() {}
}
new VisTypeMarkdownPlugin().setup({
visualizations,
data: {
expressions: {
registerFunction: (fn: any) => functionsRegistry.register(fn),
}
}
});# Conflicts: # src/legacy/core_plugins/vis_type_markdown/public/markdown_fn.ts
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
# Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json
💔 Build Failed |
|
retest |
💚 Build Succeeded |
# Conflicts: # src/legacy/core_plugins/vis_type_markdown/public/markdown_fn.test.ts
| new VisTypeMarkdownPlugin().setup({ | ||
| visualizations: visualizationsService, | ||
| data: { | ||
| expressions: { |
There was a problem hiding this comment.
Sorry for the delayed comment, but why not add this function to expressions service in data plugin?
Summary
first vis type to np shim plugin
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] 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[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately