Skip to content

Conversation

@hupfdule
Copy link
Contributor

@hupfdule hupfdule commented Apr 6, 2019

Use tabular vim plugin to align tables (if available).

Copy link
Owner

@jjaderberg jjaderberg left a comment

Choose a reason for hiding this comment

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

This looks like a nice addition.
Thanks for taking the time to contribute back. 👍
I made an initial review pass and generated some comments, change requests.


==== Aligning

When the https://github.com/godlygeek/tabular[Tabular Plugin] is
Copy link
Owner

Choose a reason for hiding this comment

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

One sentence per line. ,spl pls

Copy link
Owner

Choose a reason for hiding this comment

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

And lvl2 is to communicate "one idea per sentence".
If Tabular is highly recommended, it deserves it's own sentence.
Don't squeeze it into a parenthesis in the middle of a sentence.
Doing so interferes with the thought communicated in the surrounding sentence.

Suggest something like:

... is a popular plugin for aligning text.
The plugin is very useful for working with Asciidoc tables.
When the Tabular plugin is installed, tables can be aligned using `kbd:[+,|+]`.
This feature is limited to tables that ...


.Before
....
[%header]
Copy link
Owner

Choose a reason for hiding this comment

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

The % stuff is a nice feature but it's only noise in this context. I don't see how this adds any value for the reader. If it does it should be added in one fell swoop everywhere.


.After
....
[%header]
Copy link
Owner

Choose a reason for hiding this comment

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

as above

|===
....

If the option `g:asciidoc_table_autoalign` is set to 1 (which is set to 0
Copy link
Owner

Choose a reason for hiding this comment

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

One sentence per line.

" }}}

" Tabular ================================================== {{{
if exists(':Tabularize')
Copy link
Owner

Choose a reason for hiding this comment

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

Have you thought about the options here?
This might be the right choice.
First thought would be exists('g:tabular_loaded').
But if I have the plugin on path and don't want to load it for whatever reason, I would set that to prevent the plugin from loading.
Have you given thought to doing this particular check, or is it an "it's good enough" kind of thing?


" Tabular ================================================== {{{
if exists(':Tabularize')
" Use <Leader>| to realign tables with the Tabuliarize plugin
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
" Use <Leader>| to realign tables with the Tabuliarize plugin
" Use <LocalLeader>| to realign tables with the Tabuliarize plugin

alignment of tables is possible via kbd:[+,|+]. This applies only to tables
that use the Bar kbd:[+|+] as separator between cells.

Be aware that this only works for basic tables and certain features like
Copy link
Owner

Choose a reason for hiding this comment

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

If you really want the user to be aware, use an admonition.
In this case I think it is enough to say, simply:

The align feature does not work well for tables with cell spanning.

I am not sure what "the cell spanning will not be visualized in the asciidoc sourcecode" means.

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.

2 participants