Fix table columns breaking content #154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NickColley
approved these changes
Nov 12, 2019
NickColley
reviewed
Nov 12, 2019
CHANGELOG.md
Outdated
| @@ -1,5 +1,10 @@ | |||
| # Changelog | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| Remove the `table-layout: fixed` style from the tables in tech' docs content, mainly due to it | |||
There was a problem hiding this comment.
Not blocking by ideally the changelog should say what is fixed for users, not the details of what we've been doing internally.
There was a problem hiding this comment.
Makes sense. Does this change help?
There was a problem hiding this comment.
Great, I'd prefer if it didnt require looking in the link to understand the summary but good enough 馃憤 Thanks Tom
There was a problem hiding this comment.
That's a good point actually. I've rewritten it in this commit.
tombye
force-pushed
the
remove-table-layout-fixed
branch
2 times, most recently
from
a4a20f1
to
6ec26ec
Compare
NickColley
changed the title
table-layout to auto
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the
table-layout: fixedstyle for tables in tech' docs content, introduced as part of ffa2261.Why did we fix our table layouts?
It was introduced as part of a pull request that made 2 changes:
max-width: 40emfor<pre>blocks was removed to stop issue #100table-layout: fixedwas added to fix the problemsmax-width: 40emsolvedWhy did we constrain the width of our
<pre>tags?The comments above
max-width: 40emmention<pre>tags pushing tables to be larger than the main column (see ffa2261#diff-d7f12a4697fd2195a8c62339caee5c77L122).Why should be revert to auto-sized layouts?
1. It causes an issue with existing tables
It created issue #133 with Notify's tech' docs.
2. Removing it shouldn't effect any other existing tables
I went through the main users of this gem and found all the tables in them (thanks @NickColley for their details):
https://docs.google.com/spreadsheets/d/1OZGQifZIw0I2GXqmrp78DoI3R2sfcxn5IcFq3Y5RXqA/edit#gid=0
I can't find any tables that use
<pre>tags in table cells, just inline<code>blocks.I also tested all these tables without
table-layout: fixedand none of them displayed in ways that distorted the content or made the table wider than the main column.3. Tables that are too wide don't break the main column
Tables are wrapped in a fixed-width container which scrolls. This means that if they're wider than the main column they don't break it and users can scroll to access their content. For example:
https://www.docs.verify.service.gov.uk/legacy/build-ms/msa/#msa-tls-certificates
We probably shouldn't be using block-level code in tables
Note: this point is just my opinion 馃檪.
Middleman's markdown parser [Redcarpet uses PHP-Markdown style tables](https://github.com/vmg/redcarpet#and-its-like-really-simple-to-use. In its docs, PHP-Markdown says it allows 'span level formatting' in its tables:
https://michelf.ca/projects/php-markdown/extra/#table
There's no mention of official support for block-level formatting, like
<pre>.