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

Table styles #137

Closed
cjcenizal opened this issue Nov 10, 2017 · 9 comments
Closed

Table styles #137

cjcenizal opened this issue Nov 10, 2017 · 9 comments

Comments

@cjcenizal
Copy link
Contributor

In elastic/kibana#14471, I suggested some improvements to tables:

  • Checkboxes in a table should be lower in the hierarchy and emphasize cohesion with the row style over visual prominence
  • Selected rows need a hover state
  • A border around entire table to increase cohesion among table content and contrast against surrounding content (may not be necessary, just an idea)
  • We can give the table header a background color to make it easier to differentiate from rows (this treatment also makes it consistent with that of the Popover header, also just an idea)

image

@snide
Copy link
Contributor

snide commented Nov 10, 2017

Selected rows need a hover state

Was added in a previous PR #117. I do like that you change the border color to the off blue when selected though. We should definitely do that as it reads better. 👍

Checkboxes in a table should be lower in the hierarchy and emphasize cohesion with the row style over visual prominence

Do you mind if we address this in the separate ticket #64? Along with the switch changes here? Makes it easier to focus on the table questions your bring up.

A border around entire table to increase cohesion among table content and contrast against surrounding content (may not be necessary, just an idea)

The reason I started with the airy tables is because I'm extremely worried about people copy / pasting code and munging margins around things. The more lines added to the layout the easier it is to read that the lines are spaced weirdly. This is also important because our tables are almost always already "contained" by panels. So you end up with this weird double border (are the left/right margins matching the bottom ones...etc) around everything. It weighs things down even if it might look good on its own.

I wouldn't be opposed to making this a non-default option though. I think your styling for it looks great, I'm more just worried about it as the "works in most places" usage. For example, I would never want a small table (say in the index pattern selection page) to have this really heavy table in it, distracting from the other elements on the page, or in a spy...etc).

We can give the table header a background color to make it easier to differentiate from rows (this treatment also makes it consistent with that of the Popover header, also just an idea)

Think this looks good when the table has the borders, but weird when it doesn't. What I do admit is that our focus state for table heading selection is super ugly and needs to be fixed in both. Likely the active coloring needs to be around the text, even if the hitbox is the cell.

@cjcenizal
Copy link
Contributor Author

Thanks for the great feedback @snide! @gjones @formgeist @Adrian-J Thoughts on this?

@gjones
Copy link
Contributor

gjones commented Nov 10, 2017

Yeah I hate to be boring but I kind of echo Dave's thoughts on this. I'm weary of adding a background color to the head or introducing a table border as both things tend to add visual weight and prominence. In Cloud we have a lot of pages with multiple tables and I could forsee this approach adding up to a busy aesthetic with competing elements. I like the what Dave refers to as the airy tables because they're clean and feel light (if that makes any sense??).

I think it would be great to have as an alternate style though.

I agree with you that selected rows need a hover state and really like the colour you've chosen above.

@bevacqua
Copy link
Contributor

Maybe we can add a skin prop that defaults to keeping tables as they are, and when skin='heavy' (or some other name) we apply these suggestions?

@cjcenizal
Copy link
Contributor Author

I like the what Dave refers to as the airy tables because they're clean and feel light

I completely agree, you're both spot-on about this.

@bevacqua To your point, I'll open a separate PR specifically for this "skin" so that we discuss the props and interface there. As a side benefit this way this change will be isolated into its own commit which we can easily revert it if we don't have any use cases for it.

@bevacqua
Copy link
Contributor

Yeah, for what it's worth I like it and maybe it should even be the default prop, but easily turned off with skin={ false } or skin={ null }.

classNames({
  'euiTable--skinHeavy': skin === 'heavy'
});
EuiTable.defaultProps = {
  skin: 'heavy'
};

@formgeist
Copy link
Contributor

Late to the party, and will just be echoing Dave and Gareth's comments and suggestions 👍

@Adrian-J
Copy link

@cjcenizal I also like what we have right now, feels fresh and clean. With so many different styles it feels chaotic and difficult to read.

@snide
Copy link
Contributor

snide commented Mar 9, 2018

Closing this out. Selected row hovering and checkboxes were added / addressed. Other issues (bordering, headers) had consensus.

@snide snide closed this as completed Mar 9, 2018
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

No branches or pull requests

6 participants