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-layout: fixed. Remove 20px max-width #398

Merged
merged 2 commits into from
Feb 14, 2018
Merged

Conversation

snide
Copy link
Contributor

@snide snide commented Feb 13, 2018

Closes #389

This removes some finicky code that tried to make tables widths a bit prettier, but came with some bad drawbacks.

Moving to table-layout: fixed seems to solve most of our problems, though it might require people to hand touch cell widths a bit more (which I think is fine).

@cjcenizal
Copy link
Contributor

Just a reminder that this needs to be tested in Windows on all browsers, especially IE11 and Edge. I'm interested in seeing how this works with 1) variable column widths and 2) content which needs to wrap or be truncated in a cell.

@cjcenizal
Copy link
Contributor

I found the issue which caused me to implement the max-width originally: elastic/kibana#9929. Here's the PR which added this style: elastic/kibana#9928.

Looks like the problem was that the table ended up overflowing the container. We'll need to make sure we don't reintroduce this problem.

@snide
Copy link
Contributor Author

snide commented Feb 13, 2018

Edge is OK. Wrapping correctly happens when applied. If not applied, it breaks the normal way a table would break

table

image

@snide
Copy link
Contributor Author

snide commented Feb 13, 2018

Same goes for IE11. Breaks happen when wrapping truncation is not turned on. A lot of these smaller width scenarios are going to be solved better by a more comprehensive responsive table PR.

image

image

@snide
Copy link
Contributor Author

snide commented Feb 13, 2018

But at least from what I can see, there are no adverse effects to this PR other than that it might require the consumer to adjust table widths on their own to make the tables themselves a little prettier (for example above, you'd want to adjust the orders of magnitude column to have a max-width or something).

Ultimately though, the consumer should truncate text as they feel the need to. Beyond that we need a better solution for responsive tables.

@chrisronline
Copy link
Contributor

I don't much to add, but just echoing CJ's concerns. I'd be wary of a regression, but happy if you both feel we're good.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM!

@snide snide merged commit aff62f7 into elastic:master Feb 14, 2018
@snide snide deleted the table/width branch February 14, 2018 22:54
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.

Fix long name overlap in Table
3 participants