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.

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

Merged
merged 17 commits into from
Jun 15, 2016

Conversation

tylersmalley
Copy link
Contributor

@tylersmalley tylersmalley commented May 25, 2016

Closes #7135

@tylersmalley tylersmalley force-pushed the 7135-management branch 3 times, most recently from f2ada52 to 725fab9 Compare May 26, 2016 21:28
@tylersmalley tylersmalley changed the title WIP: Changes settings to management [WIP] Changes settings to management May 26, 2016
@tylersmalley tylersmalley force-pushed the 7135-management branch 5 times, most recently from 8439192 to 1b73263 Compare June 1, 2016 15:51
sections.register('kibana', {
display: 'Kibana',
order: 20,
});
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

@tylersmalley tylersmalley changed the title [WIP] Changes settings to management Changes settings to management Jun 1, 2016
@epixa
Copy link
Contributor

epixa commented Jun 1, 2016

Can you make "Management" a link in the breadcrumbs?

@epixa
Copy link
Contributor

epixa commented Jun 1, 2016

Under Kibana, can you update it from "Objects" to "Saved Objects"? No reason for succinctness here, we have plenty of space!

@epixa
Copy link
Contributor

epixa commented Jun 1, 2016

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.

@tylersmalley
Copy link
Contributor Author

@epixa

Can you make "Management" a link in the breadcrumbs?

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.

Under Kibana, can you update it from "Objects" to "Saved Objects"? No reason for succinctness here, we have plenty of space!

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.

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.

Makes sense, especially with it being under Kibana.

@tylersmalley tylersmalley assigned tylersmalley and unassigned epixa Jun 1, 2016
@tylersmalley
Copy link
Contributor Author

tylersmalley commented Jun 1, 2016

Per the conversation with @epixa @alt74:

  • Index patterns links to the default index pattern
  • Use a single column, full width layout for the management landing page to improve initial UI.
  • Toggled info should turn icon black
  • "Management" in breadcrumbs should link back to landing page
  • More padding above info text to reflect design
  • Remove version from Kibana info

@tylersmalley tylersmalley force-pushed the 7135-management branch 3 times, most recently from 5401d8d to ec8cbef Compare June 6, 2016 20:13
@tylersmalley
Copy link
Contributor Author

@epixa I have complete all but linking of Management in the breadcrumbs. There was a couple of ways I see us accomplishing this:

  • The first one, was what I mentioned earlier. Using the ManagementSection, walk down the tree to produce the breadcrumbs. After doing this, it didn't feel right as breadcrumbs are used elsewhere.
  • The second is to expand on the existing breadcrumbs, and add the functionality to all of Kibana. This does appear to be currently possible as angular-router doesn't have a parent-child relationship. To continue on this, I have been investigating moving to ng-router.

Interested in your thoughts.

@tylersmalley tylersmalley assigned epixa and unassigned tylersmalley Jun 6, 2016
@epixa
Copy link
Contributor

epixa commented Jun 6, 2016

@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.

@epixa
Copy link
Contributor

epixa commented Jun 6, 2016

Since my time is limited for the next few days, grab another person to review this simultaneously (there is multi-assign now).

Tyler Smalley added 12 commits June 14, 2016 16:43
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>
@tylersmalley
Copy link
Contributor Author

@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.

@tylersmalley
Copy link
Contributor Author

@Bargs

There are a lot of discrepancies between the styles in the current pages vs the Invision prototype, I assume that's out of scope for this PR?

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']);
Copy link
Contributor

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.

@Bargs
Copy link
Contributor

Bargs commented Jun 15, 2016

@tylersmalley I left just two minor follow up comments, I'll link to them here since this thread is getting really long.

#7284 (comment)
#7284 (comment)

Tyler Smalley added 2 commits June 15, 2016 08:54
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@Bargs
Copy link
Contributor

Bargs commented Jun 15, 2016

LGTM 👍

@lukasolson
Copy link
Member

WOOOOT 😲⭐🍠💎🐨 🎉

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@tylersmalley
Copy link
Contributor Author

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.

@tylersmalley tylersmalley merged commit 643ae1b into elastic:master Jun 15, 2016
@Bargs Bargs mentioned this pull request Jun 16, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Changes settings to management

Former-commit-id: 643ae1b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants