feat: support text wrapping #6
Conversation
There was a problem hiding this comment.
This looks pretty good so far, thanks for starting on it. Only a few implementation comments.
Could you also run standard --fix*? That will take care of a lot of the nit stuff, then in the future you can run npm run lint to make sure you're conforming to style.
* probably have to do something like ./node_modules/bin/standard --fix if you don't have standard installed globally. maybe I'll add an npm script to do that along with lint
index.js
Outdated
|
|
||
| for (let i = 0;i < width;i++){ | ||
| const cells = values[i] | ||
| let u = 1 |
There was a problem hiding this comment.
I'm usually fine with i as a variable since it's pretty universally viewed as an index, but otherwise will try to be more descriptive. Maybe change u to cell?
There was a problem hiding this comment.
I agree, but I'm not sure cell would be strictly correct. These are indexes, or rather coordinates, in a matrix, not the cell itself (which would be values[i][u] in this case).
I was thinking about h and v for horizontal and vertical respectively, what do you think about that?
Also cell in the first loop should probably be cells as well, as it's the same as in this loop.
index.js
Outdated
| @@ -112,3 +168,7 @@ function padStart (what, target, start) { | |||
| function padEnd (what, target, start) { | |||
| return what + repeat(' ', target - what.length) | |||
| } | |||
|
|
|||
| function max (a, b) { | |||
There was a problem hiding this comment.
Why use this instead of Math.max?
There was a problem hiding this comment.
Originally because I used it in a reduce (where the "only two arguments" is important). I just kept it for the looks.
I'm fine with dropping it.
index.js
Outdated
| }, options) | ||
| stringify: v => typeof v === 'undefined' ? '' : String(v), | ||
| }, options, { | ||
| wrap: Object.assign({ |
There was a problem hiding this comment.
We should think about the CLI here. It'd be easier to handle these options at the CLI if they aren't in a nested object, so using wrapWidth and wrapGutters would need less tomfoolery.
Thoughts?
There was a problem hiding this comment.
Yeah, I added a gfm (GitHub Flavored Markdown) option to the CLI to quickly disable wrapping, replace newlines with breaks and pipes (|) with \|, so it doesn't really matter as we have to "spell out" the options anyway.
readme.md
Outdated
| ```js | ||
| tablemark([ | ||
| { star: false, name: 'Benjamin' }, | ||
| { star: true, name: 'Jet Li' }, |
readme.md
Outdated
| ```js | ||
| tablemark([ | ||
| { star: false, name: 'Benjamin' }, | ||
| { star: true, name: 'Jet Li' }, |
readme.md
Outdated
|
|
||
| The `columns` array can either contain objects, in which case their | ||
| `name` and `align` properties will be used to alter the display of | ||
| the column in the table, or any other type which will be coerced | ||
| to a string if necessary and used as a replacement for the column | ||
| name. | ||
|
|
||
| ## text wrapping | ||
|
|
||
| To output valid [GitHub Flavoured Markdown](https://github.github.com/gfm/) a |
There was a problem hiding this comment.
I'd prefer Flavored here 😆 That's also what GitHub uses in those docs.
| const first = new Array(width) | ||
| let height = 1 | ||
|
|
||
| for (let h = 0; h < width; h++) { |
There was a problem hiding this comment.
Let's compress a bit from here to line 110, the empty lines here remind me of double spacing papers for essays back in the day 😆
for (let h = 0; h < width; h++) {
const cells = values[h] = split(columns[h], widths[h])
if (cells.length > height) height = cells.length
first[h] = pad(alignments[h], widths[h], cells[0])
}
if (height === 1) return line(first, true)
const lines = new Array(height)
lines[0] = line(first, true)
So, as you might be able to see, I ended up focusing defaults on producing valid Markdown output.
Btw: don't release anything, I have some more coming up.. also for the CLI.