-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
Vislib replacement toggle #56439
Vislib replacement toggle #56439
Conversation
Looks like you've got some typos in your banner. Also, is it necessary for this banner to If no to B, it should be a Toast that disappears, and if no to A, then there probably shouldn't be any UI message but maybe just a simple console warning. |
@cchaos Thanks on the typos. 🤦♂ This config is not going to be documented so I really only want this banner in case someone by chance, enables this before it's fully compatible with the current vislib charts. Being that there might be missing features while it's being fully converted but the code still lives in master. The idea would be to remove the banner once there is feature parity. In that case are you ok with it being persisted? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed a few small things. Not sure about the banner - do we just want to show it all the time?
.i18nrc.json
Outdated
"visTypeVislib": "src/legacy/core_plugins/vis_type_vislib", | ||
"visTypeVislib": [ | ||
"src/legacy/core_plugins/vis_type_vislib", | ||
"src/legacy/core_plugins/vis_type_xy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use visTypeXy
as i18n prefix for the new plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ expressions, visualizations, charts }: VisTypeXyPluginSetupDependencies | ||
) { | ||
const [{ overlays }] = await core.getStartServices(); | ||
const mountBanner: VisTypeXyDependencies['mountBanner'] = mountBannerOnce(overlays); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the banner isn't mounted anymore because it's not actually overwriting a visualization. That's fine for me in general, just wanted to make sure it's happening on purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I figured just leaving it in their until the first replacement visualization is added and I'll mount it in there.
@@ -39,6 +39,7 @@ import { | |||
createGoalVisTypeDefinition, | |||
} from './vis_type_vislib_vis_types'; | |||
import { ChartsPluginSetup } from '../../../../plugins/charts/public'; | |||
import { ConfigShema as VisTypeXyConfigShema } from '../../vis_type_xy'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo shema
instead of schema
- got autocompleted to a few other places as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit on the fence regarding the banner too. I see that it might make sense that we're notifying the users while we're only having this on master (i.e. before we finished building everything up), that some charts might behave differently. But not sure if a banner is the right way for this? Since I assume we'll mainly assume developers will use this flag before we don't think we've moved over all functionality, I'd assume a console log might be enough? The banner might also disturb (visual) testing other features for users during development. Also since we ask in bug reports for the console output, I think putting something there would help us see if a user that's not currently developing "accidentally" switched on that flag, and is trying to report bugs for the new elastic charts type? |
|
||
const visTypeXyPluginInitializer: LegacyPluginInitializer = ({ Plugin }: LegacyPluginApi) => | ||
new Plugin({ | ||
id: 'vis_type_xy', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use camelCase id here too directly, since we might need to once we're moving to the new platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in 4ff43c5. It looks like it shows a JSON schema warning using camelCase but runs fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, did not test again
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* master: (42 commits) Move kuery_autocomplete ⇒ NP (elastic#56607) [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595) [Discover] Inline angular directives only used in this plugin (elastic#56119) [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011) [SIEM] Enable flow_target_select_connected unit tests (elastic#55618) Start consuming np logging config (elastic#56480) [SIEM] Add eslint-plugin-react-perf (elastic#55960) Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613) Add `getServerInfo` API to http setup contract (elastic#56636) Updates Monitoring alert Jest snapshots Kibana property config migrations (elastic#55937) Vislib replacement toggle (elastic#56439) [Uptime] Add unit tests for QueryContext time calculation (elastic#56671) [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts Upgrade EUI to v18.3.0 (elastic#56228) [Maps] Fix server log (elastic#56679) [SIEM] Fixes FTUE when APM node is present (elastic#56574) [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563) Update EMS API urls for production (elastic#56657) Ability to delete alerts even when AAD is out of sync (elastic#56543) ...
* Add new vislib replacement plugin shell * Add config to toggle new vislib replacement
* Add new vislib replacement plugin shell * Add config to toggle new vislib replacement
* Add new vislib replacement plugin shell * Add config to toggle new vislib replacement
Summary
visTypeXy
pluginWhen enabled, all available replacement
vislib
visualizations will be registered undervis_type_xy
, not undervis_type_vislib
.If the plugin is enabled, a
console.warn
will indicate potentially negative configuration mutations.Checklist
For maintainers