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

Homepage / index pattern design cleanup #16005

Merged
merged 6 commits into from
Jan 12, 2018
Merged

Conversation

snide
Copy link
Contributor

@snide snide commented Jan 11, 2018

Replaces KUI with EUI components in the homepage / install / directory pages. This makes it look closer to the original mocks. Also fixed some small issues I saw in the EUI implementation of index patterns.

@snide snide requested a review from nreese January 11, 2018 21:49
@@ -56,7 +57,7 @@ export function Home({ addBasePath, directories }) {
return (
<div className="kuiVerticalRhythm">
<KuiCardGroup>
<KuiCard style={cardStyle}>
<KuiCard style={cardStyle} className="euiPanel">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class works for now. I'll replace with a EuiCard component once I have one built.

</EuiText>
</EuiTextColor>
<EuiSpacer size="s" />
<a href="/app/kibana#/management/kibana/index" className="euiButton euiButton--primary euiButton--small">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EuiButton doesn't have an href ability yet, so I'm manually building the HTML. We should replace this with a component later.

@snide
Copy link
Contributor Author

snide commented Jan 11, 2018

cc @formgeist @alexfrancoeur who were interested in this.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like the jest tests for tutorial needs to have updated snapshots.

@cchaos
Copy link
Contributor

cchaos commented Jan 11, 2018

I'm not a fan that the breadcrumbs move within the page layout. Can we always keep the breadcrumbs tight to the top left but the page title and stuff can get centered?

screen shot 2018-01-11 at 17 31 00 pm

screen shot 2018-01-11 at 17 31 09 pm

:focus {
@include focus;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjcenizal Don't need you to review this whole PR, but can you think of any reason this approach for the focus stuff is a bad idea?

@formgeist
Copy link
Contributor

I'm with @cchaos on the breadcrumb placement shifting from the directory pages to instructions page. Wondering if "Home" breadcrumb label should be added to the Home page too. But perhaps the link colour shouldn't be blue, but rather grey and turn darker with underline on hover like in the tabs. Much like the new header breadcrumb.

I'd also suggest adding some more space between the headline and the first three solutions boxes. It all feels a little cramped now. Additionally, I'd not make the index pattern text block on top of each other. Looked better before IMO.

screenshot 2018-01-12 09 36 56

Awesome to see the new tabs and panels in the directory pages though 🙌

@snide
Copy link
Contributor Author

snide commented Jan 12, 2018 via email

@formgeist
Copy link
Contributor

I understand - perhaps the extra space underneath that section might help it feeling less boxed up in the corner.

@formgeist
Copy link
Contributor

Just noticed the spacing feels a little off in the instructions. The spacing from the step title to content is more than from one step to another. I'd turn this around - add less space from title to to content, possibly add the extra space to the bottom of the content to the next step.

screenshot 2018-01-12 09 49 45

@snide
Copy link
Contributor Author

snide commented Jan 12, 2018 via email

@alexfrancoeur
Copy link

loving this @snide!

I noticed that with the updates to the card's in the list view (or maybe this was the case before) we are not highlighting as we tab through. It's tabbing, but the blue border indicating the element I'm on is now missing. The tabs (All, Metrics, Logs, Security Analytics) also don't seem to highlight light blue as the tabs do in the tutorial view when you navigate via keyboard.

jan-12-2018 06-52-28

Probably out of scope for this PR, but things get weird when you go full screen in a large monitor. What are the thoughts around allowing for a maximum width for the cards / sections here and then just centering the content at some point? I realize this may be resolved with EUI but thought I'd throw it out there.

screen shot 2018-01-12 at 6 51 13 am

Also, big +1 on the breadcrumb. I mentioned this a few times in my original reviews of the PR. Seems odd to me that we shift from top left to center. While the breadcrumb is honestly a bit different with every application, it's at least consistently placed in the top left

This isn't too big of a deal and may be more for @nreese but I found it interesting that the breadcrumb didn't bring me back to the exact path I took. If you look at the gif below, I go through metrics here but then returning to add data brings me to the All tab. Again, not a priority but I just found it odd.

jan-12-2018 07-01-33

Can't wait to see the icons in here too! From https://github.com/elastic/dev/issues/844#issuecomment-357170834 it sounds like we may possibly need a few extra in 6.2. Though I imagine we can introduce these after FF if necessary.

Love that this is now underlining the selected tab. It looked a bit odd just being white underneath the tab you were in.

screen shot 2018-01-12 at 7 06 31 am

I've mentioned this a few times and would be interested in designs take here, but all URL's currently redirect you away from Kibana. I think the better user experience here would be to open tabs instead. Thoughts on general workflow and if we agree, addressing in this PR? Also the more I think about it - what's the best experience here from an accessibility perspective? I'd still lean towards new tab.

All in all, the new design changes, spacing, margins and numbered / bulleted steps look great.

@nreese
Copy link
Contributor

nreese commented Jan 12, 2018

@alexfrancoeur URLs should now open a new tab when clicked - #15995

@alexfrancoeur
Copy link

@nreese awesome! appreciate the quick turnaround on that, I think it will drastically improve the experience here

@snide
Copy link
Contributor Author

snide commented Jan 12, 2018

OK. Think I got most of everyones issues...

  • Max-width set on all home/install pages. This forces the breadcrumbs to align against the content at all times. I also went through and converted as much of this to EUI as I could so that the positions of titles / breadcrumbs and the like don't shift too much.
  • Spacing added to the bottom of steps to give some offset to single-line / button combo rows.
  • Synopsis bit refactored so it allows for better focus states.

I've migrated as much of this as I could to using pure EUI components now. Still some buttons that I can't convert till we fix elastic/eui#24

@alexfrancoeur
Copy link

just pulled down, lgtm dave

@snide snide merged commit c5001b1 into elastic:master Jan 12, 2018
@snide snide deleted the eui/cleanup branch January 12, 2018 23:59
snide added a commit to snide/kibana that referenced this pull request Jan 13, 2018
Rebuilds most of the homepage using EUI components. Adjusts styles to be closer to mocks.

Conflicts:
	src/core_plugins/kibana/public/home/components/tutorial/introduction.js
snide added a commit to snide/kibana that referenced this pull request Jan 13, 2018
Rebuilds most of the homepage using EUI components. Adjusts styles to be closer to mocks.
snide added a commit that referenced this pull request Jan 13, 2018
Rebuilds most of the homepage using EUI components. Adjusts styles to be closer to mocks.
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.

5 participants