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

Add markdown-insert-table #369

Closed
wants to merge 2 commits into from

Conversation

shigemk2
Copy link
Contributor

Description

  1. Define column number
  2. Define row number
  3. Define align type (left, right or center)
  4. Define headers

Related Issue

Nothing.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have updated the documentation in the README.md file if necessary.
  • I have added an entry to CHANGES.md.
  • I have added tests to cover my changes.
  • All new and existing tests passed (using make test).

1. Define column size
2. Define row size
3. Define align type (left, right or center)
4. Define headers
Copy link
Owner

@jrblevin jrblevin left a comment

Choose a reason for hiding this comment

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

Thanks for this patch! Great idea. I noticed a couple of things to fix before I can commit. The keybinding is the main one, but there are a few suggestions to make it more obvious/easier to use also.

Also, thanks for including tests!

markdown-mode.el Outdated
@@ -5404,6 +5404,7 @@ Assumes match data is available for `markdown-regex-italic'."
(define-key map (kbd "C-c C-t s") 'markdown-insert-header-setext-2)
(define-key map (kbd "C-c C-t t") 'markdown-insert-header-setext-1)
(define-key map (kbd "C-c C-i") 'markdown-insert-image)
(define-key map (kbd "C-c C-t") 'markdown-insert-table)
Copy link
Owner

Choose a reason for hiding this comment

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

C-c C-t is actually a prefix to heading (titling) commands (which can be seen a couple of lines above). We'd need to find a new keybinding. Perhaps C-c C-s t, which would be under markdown-mode-style-map.

markdown-mode.el Outdated
@@ -5528,6 +5529,8 @@ See also `markdown-mode-map'.")
:enable (markdown-table-at-point-p)]
["Insert Column" markdown-table-insert-column
:enable (markdown-table-at-point-p)]
["Insert Table" markdown-insert-table
:enable (markdown-table-at-point-p)]
Copy link
Owner

Choose a reason for hiding this comment

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

I think the predicate should be(not (markdown-table-at-point-p)), which means this menu item is only enabled when there is not a table at the point.

markdown-mode.el Outdated
(cond ((equal align-type "left") (setq content ":---"))
((equal align-type "right") (setq content "---:"))
((equal align-type "center") (setq content "---"))
(t (user-error "Speficy align-type: left, right or center")))
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps centered should be the pass-through default?

markdown-mode.el Outdated
(interactive)
(let ((table-column (string-to-number (read-string "column size: ")))
(table-row (string-to-number (read-string "row size: ")))
(align-type (read-string "align type: "))
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe a helpful tip in the prompt such as "(left, right, center (default)):"

README.md Outdated
@@ -579,6 +579,14 @@ can obtain a list of all keybindings by pressing <kbd>C-c C-h</kbd>.
correctly when calculating column widths, however, columns
containing hidden markup may not always be aligned properly.

<kbd>C-c C-t</kbd> (`markdown-insert-table`) is a general command for
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for updating the docs. Please update this line with the new keybinding (see below).

README.md Outdated
- Specify row size.
- Specify column size.
- Specify table align: right, left or center.
- Specify header contetnts.
Copy link
Owner

Choose a reason for hiding this comment

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

Spelling: "contents"

- Change keybinding from `C-c C-t` to `C-c C-s t`.
- Fix typo in README.
- Delete markdown-insert-table's enable condition in markdown-mode-menu.
- Set center align as default and fix test codes.
@shigemk2
Copy link
Contributor Author

@jrblevin Thank you for your comments. I fixed some codes following your comments.

@jrblevin
Copy link
Owner

Thanks for updating. I applied this as 5252b71 with a couple of additional minor changes: I removed the duplicate keybinding (it's only needed in the style keymap under C-c C-s) and I also updated CHANGES.md. Thanks again for submitting this.

@jrblevin jrblevin closed this Oct 29, 2018
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