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

Single page pagination, button reset, table header sorting, codeblock styling #298

Merged
merged 6 commits into from
Jan 12, 2018

Conversation

snide
Copy link
Contributor

@snide snide commented Jan 12, 2018

Fixes #297, #268 and #289, #300, #299

Small stylistic changes.

Pagination no long shows a single item

Table heads no longer show vague "is sortable" state. Things just have hover states or not. Only one sort shows.

Codeblock sizing increasing with additional padding per feedback form designers

image

@snide snide requested a review from cjcenizal January 12, 2018 07:54
@snide snide requested review from cchaos and removed request for cjcenizal January 12, 2018 07:59
This was referenced Jan 12, 2018
@cchaos
Copy link
Contributor

cchaos commented Jan 12, 2018

Was there an idea to also disable the Previous/Next pagination links instead of removing them?

@@ -75,7 +75,8 @@
display: inline-block;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove vertical-align: middle from .euiCodeBlock--inline it will align it to the baseline.

screen shot 2018-01-12 at 11 36 47 am

vs.

screen shot 2018-01-12 at 11 36 41 am

@@ -75,7 +75,8 @@
display: inline-block;
white-space: pre;
color: $euiTextColor;
font-size: 75%; // 1
font-size: 90%; // 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that it's still slightly smaller, but this is a much better scale.

@@ -99,6 +99,7 @@ button {
color: inherit;
font-size: inherit;
border-radius: 0;
text-align: left;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this have any ill cascading effects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't in EUI, but it might in the nest of Kibana. I'm gonna move it over to EuiLink which was the primary sore spot (that onClick turns EuiLink into buttons, which shouldn't have alignment). That's probably safer.

@snide
Copy link
Contributor Author

snide commented Jan 12, 2018

@cchaos Addressed.

@cchaos
Copy link
Contributor

cchaos commented Jan 12, 2018

Any thoughts on my previous comment or leave that for another PR?

@cjcenizal
Copy link
Contributor

Can we find a way to visually communicate that a column is sortable or not? It kind of sucks for the user to need to hover on something to find out whether it's clickable. Better to know something is clickable before you try to click it.

@cjcenizal
Copy link
Contributor

Or maybe we can find a way to visually communicate the column is not sortable. Maybe by graying out the column name slightly?

@cchaos
Copy link
Contributor

cchaos commented Jan 12, 2018

@cjcenizal We talked about it a bunch #268 and finalized on hover being the indicator. But your second idea could help as well – just a slightly lighter gray header. Not sure it's necessary for this PR.

@snide
Copy link
Contributor Author

snide commented Jan 12, 2018

Talked over slack. Merging for now, but no one is super happy with the header styles. We'll need to fix that later. @cchaos we'll also deal with the separate pagination issue later. Feel free to set up a PR on that if you have ideas.

@snide snide merged commit cb33008 into elastic:master Jan 12, 2018
@snide snide deleted the bugs/small branch January 12, 2018 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants