Create "chromeConfigControls" for plugins to add chrome config sections #6151
Conversation
Each chromeConfigControl has a navbar component and a config component.
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'. |
|
This needs to be rebased on master or have master merged into it. |
…me-ui Conflicts: src/ui/public/chrome/context.js
|
I merged master into it. |
|
jenkins, test it |
| @@ -56,6 +56,8 @@ | |||
| <!-- /Full navbar --> | |||
| </nav> | |||
|
|
|||
| <div kbn-chrome-config-controls> | |||
There was a problem hiding this comment.
nitpick: can we just make this a custom element, <kbn-chrome-config-controls>
|
This is looking great @trevan |
Change kbn-chrome-config-controls to an element Import the config section html instead of require
|
@spalger, I updated per your comments. I also added tests for the new directive. And I modified the append nav directive to merge the items from the nav registry and the config registry so you can control their order. |
|
jenkins, test it |
|
This LGTM. Opened #6224 which merged this with master. This is ready for another set of eyes. Working on tracking down a candidate. |
|
Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run; then say 'jenkins, test it'. |
|
@spalger, I noticed that the autorefresh functionality was broken with these changes so I've merged in master and fixed the issue. |
|
jenkins, test it |
| }), | ||
|
|
||
| ...configs.map(function (config) { | ||
| const configHtml = `<render-directive definition="configs['${config.name}'].navbar"> |
There was a problem hiding this comment.
Not sure how I missed this the first time, but we should probably take a tad-bit more care with how we build configHtml
There was a problem hiding this comment.
Well, I guess we have to trust the template value, but I would feel better if we changed the injection of config.name to something like:
`<render-directive definition="configs.byName['${JSON.stringify(config.name)}'].navbar">There was a problem hiding this comment.
The JSON.stringify didn't work since it add double quotes which messed up the html parsing. I instead add a regex piece in the registry that forces the names to only allow a-z, A-Z, or 0-9.
|
Awesome, thanks for the updates @trevan! |
|
Assigned to @lukasolson to get a second set of eyes on the changes |
|
Looks like we need master merged again... :) |
|
@lukasolson, I merged master |
|
Alright, so with the updated coming in feature/redesign I think we need to put this on hold momentarily. Sorry @trevan. Once we get feature/design merged we can find a way to get this into there. It shouldn't be too hard, but since the chrome is now on the left of the screen and the taskbar is implemented by each application, rather than injected by the chrome, we will need to think about how to implement this there. |
|
@spalger, ok. I still have a use for a control next to the timepicker and I don't want it to just be a static string either. |
|
This is no longer blocked, though maybe you were able to achieve what you wanted without this, either way @trevan, would you like to continue this? |
|
@trevan i understand you're busy, but due to a lack of feedback, I'm going to close this PR. If you would like to submit another one under the same branch it is always welcome! |
The timepicker config and navrbar section has been moved over to this registry so you can get an idea of what it would look like. I also removed the chorome/context.js since that is now inside of timepicker/toggle.js.
I did not convert the app switcher since that appears to be going away in the redesign.
I have not yet added unittests for the new "kbnChromeConfigControls" directive but all the other tests should pass.
This closes #6136