Add missing border styles in default stylesheet

[leif81]

This is for asciidoctor/asciidoctor#569.

I've never used sass before and I'm by no means a CSS expert so bare with me. It took a little trial and error to get things looking OK. I'm pretty happy with how it turned out and tables look a heck of a lot better in print view too. Other than copying the styles verbatim from epub3.css, there are a few small changes worth pointing out here:

1) I added a CSS selector block not found in epub3.css to avoid the grid-all class from adding a border on the right side of the table. My guess was that the grid-all class was intended to only style inner borders and the frame-all class to only style outer ones? The issue this addresses was noticeable when frame-none and grid-all were used in combination. If this is desired we may want to apply this fix to epub3.css too.

table.grid-all td:last-child { border-right-width: 0; }

2) I tried to be clever and use the sass variables $table-border-highlight-size and $table-border-highlight-color. One consequence of this is that the color is #dddddd rather than #80807F if I had copied verbatim from the epub3.css.

3) Lastly I removed the original table.grid-all selector block that among other things was rounding the table corners. I noticed it wasn't used in epub3.css and it was causing me some grief so I got rid of it.

Let me know what you think. If any of this requires fixing up let me know.

I suspect once this is merged we'll want a PR that copies the generated asciidoctor.css into the asciidoctor project?

[mojavelinux]

Nice work! I think you did an excellent job of figuring out SASS! Table borders are one of the hardest places to start, so an extra :+1: for that.

The table-border-highlight-color and table-border-highlight-side were actually part of an experiment and I think we should drop them now that we have proper borders. We should just use table-border-color and table-border-size. I also agree we should drop the border radius stuff too. It only adds complexity to the stylesheet.

I made a few tweaks to catch some corner cases. The following styles work as expected in all my tests. Could you update the pull request?

// NOTE .grid-* selectors must be defined before .frame-* selectors in order for styles to cascade properly
table.tableblock,
th.tableblock,
td.tableblock {
  border: 0 solid $table-border-color;
}

table.grid-all th.tableblock,
table.grid-all td.tableblock {
  border-width: 0 $table-border-size $table-border-size 0;
}

table.grid-all tfoot > tr > th.tableblock,
table.grid-all tfoot > tr > td.tableblock {
  border-width: $table-border-size $table-border-size 0 0;
}

table.grid-cols th.tableblock,
table.grid-cols td.tableblock {
  border-width: 0 $table-border-size 0 0;
}

table.grid-all * > tr > .tableblock:last-child,
table.grid-cols * > tr > .tableblock:last-child {
  border-right-width: 0;
}

table.grid-rows th.tableblock,
table.grid-rows td.tableblock {
  border-width: 0 0 $table-border-size 0;
}

table.grid-all thead:last-child > tr > th.tableblock,
table.grid-rows thead:last-child > tr > th.tableblock {
  border-bottom-width: 0;
}

table.grid-all tbody > tr:last-child > th.tableblock,
table.grid-all tbody > tr:last-child > td.tableblock,
table.grid-rows tbody > tr:last-child > th.tableblock,
table.grid-rows tbody > tr:last-child > td.tableblock {
  border-bottom-width: 0;
}

table.grid-rows tfoot > tr > th.tableblock,
table.grid-rows tfoot > tr > td.tableblock {
  border-width: $table-border-size 0 0 0;
}

table.frame-all {
  border-width: $table-border-size;
}

table.frame-sides {
  border-width: 0 $table-border-size;
}

table.frame-topbot {
  border-width: $table-border-size 0;
}

[leif81]

Awesome. Do you prefer me squashing and force pushing changes to PR's or just pushing a new commit with the fix so we can see the history?

[leif81]

Btw I noticed a couple possibly unintended side effects in your change:

• This drops the frame-topbot class, was that intended?
• When frame-none+ grid-all are set the table bottom is bordered. I suspect that's not intended because when frame-none + grid-rows is used it doesn't do that? I expect the intention of grid-all to be grid-all = grid-rows + grid-cols?

[leif81]

Btw hope that's not weird for me to set you as the author for that second patch. Still trying to figure out the best way to make fixes to a PR based on feedback.

[leif81]

Ok take a peek at those two commits that address the issues I commented on above. You're right, getting these table border combinations just right is making my head swim :)

[mojavelinux]

This drops the frame-topbot class, was that intended?

Copy-paste error. I just updated the code block in my comment to include it.

[mojavelinux]

Whenever I update a pull request, I usually just force an amended commit. I also keep the original author, even if it incorporates changes suggested by others in the comments. The way I see it, the person who sent the pull request is driving the change and therefore gets to be the author :)

[leif81]

Cool, in the future I'll amend and force push the commits on PR's.

I've got the frame-topbot in the PR now.

I'm very happy with it all. Anything else before it's merged?

[mojavelinux]

When frame-none+ grid-all are set the table bottom is bordered. I suspect that's not intended because when frame-none + grid-rows is used it doesn't do that? I expect the intention of grid-all to be grid-all = grid-rows + grid-cols?

Thanks. I completely overlooked that case. I updated the code block to add grid-all on the tr:last-child styles for grid-rows. I also disable the bottom border if the table only has a header row.

[mojavelinux]

Btw, you caught and corrected the two issues you mentioned in the same way as I suggested in my comments, so the commit is sound. You just need to add the following rules:

table.grid-all thead:last-child > tr > th.tableblock,
table.grid-rows thead:last-child > tr > th.tableblock {
  border-bottom-width: 0;
}

Also, go ahead and remove the commented rules and squash the commit. Then we'll merge!

[leif81]

Ok I've made those changes and squashed it up! Merge when ready! :ship:

[mojavelinux]

Awesome! Nice work! I'll merge that shortly and migrate the updates to Asciidoctor core.