-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes settings to management #7284
Conversation
f2ada52
to
725fab9
Compare
8439192
to
1b73263
Compare
sections.register('kibana', { | ||
display: 'Kibana', | ||
order: 20, | ||
}); |
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 would like to discuss the interface for registering a section. Currently, it's required that you register a section prior to adding items to it. That has proved to be challenging as everything is implemented as a plugin with no guaranteed order. One thought is by getting a section will create it if it doesn't exist. You could then set details like order and display name elsewhere. Because of this challenge, I am currently initializing the core sections here, any empty section will be ignored in the UI.
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.
Personally, I'm 100% alright with this. Trying to make a completely generic interface that can scale across any number of categories with any number of links is downright impossible to do well. I think it makes sense for the management app to specifically control which categories can exist.
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 think it makes sense for the management app to specifically control which categories can exist.
I don't agree with that in the long term, but I do agree that we don't need to solve it in this PR.
1b73263
to
9a3b7a8
Compare
Can you make "Management" a link in the breadcrumbs? |
Under Kibana, can you update it from "Objects" to "Saved Objects"? No reason for succinctness here, we have plenty of space! |
What do you think about "Settings" instead of "Advanced"? We used to call them "advanced settings" to try to differentiate them from the "settings" app as a whole, but that's obviously no longer necessary. |
Yeah, I can do that. Currently the bread-crumbs directive simply splits on the path. I was throwing around the idea of using the paths to correspond to the id's of the nested ManagementSections. Since the sections can have a URL it would solve for that. Also, we could not provide a URL for things like "Kibana" which should not be linked.
Will do. I am realizing that the naming was previously kept to one word as the page names/breadcrumbs coming from the paths were simply using capitalize with CSS.
Makes sense, especially with it being under Kibana. |
Per the conversation with @epixa @alt74:
|
5401d8d
to
ec8cbef
Compare
@epixa I have complete all but linking of Management in the breadcrumbs. There was a couple of ways I see us accomplishing this:
Interested in your thoughts. |
@tylersmalley Let's get this merged and deal with that as a follow up. A solution across all of Kibana is important for consistency's sake, so consistency not linking is better than a once-off. |
Since my time is limited for the next few days, grab another person to review this simultaneously (there is multi-assign now). |
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
f3952fc
to
56d03d9
Compare
@Bargs & @lukasolson: Thank you two for your feedback on this PR. All of your comments should have been addressed in the subsequent commits and it should be passing all tests. |
There were discussions with @alt74 and @epixa regarding some changes to the management page which were not updated within Invision, like the single column layout. The design changes for the individual pages (edit, create, status, etc) will be made in subsequent PR's. |
|
||
it('is indexed on id', () => { | ||
expect(section.items.byId).to.be.an('object'); | ||
expect(Object.getOwnPropertyNames(section.items.byId)).to.eql(['three', 'one', 'two']); |
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.
getOwnPropertyNames still doesn't guarantee an ordering. You might need to manually sort the results before comparing.
@tylersmalley I left just two minor follow up comments, I'll link to them here since this thread is getting really long. |
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
89794d0
to
3ec1c51
Compare
LGTM 👍 |
WOOOOT 😲⭐🍠💎🐨 🎉 |
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
I realized on an index deletion we were previously redirecting to the creation view. I made this small change and am awaiting tests to pass, then will merge. |
Changes settings to management Former-commit-id: 643ae1b
Closes #7135