asciidoctor / atom-language-asciidoc

⚛ AsciiDoc language package for the Atom editor.
https://atom.io/packages/language-asciidoc
MIT License
42 stars 20 forks source link

Complete rewrite of snippets. #98

Closed ldez closed 8 years ago

ldez commented 8 years ago

Complete rewrite of snippets.

Notes:

Fix #23

nicorikken commented 8 years ago

What about the / prefix suggested in the issue discussion?

ldez commented 8 years ago

useful only for character . I didn't find any edge effect with others chars.

It seems more natural to me in writing.

nicorikken commented 8 years ago

I'd like to give this some personal 'usability testing' before merging, but just the code looks nice! I'm particularly interested in the usability of the 'a' prefix solution. I didn't know about the descriptionMoreURL but that is a very nice addition. I hope this can be included in 1.2.0 as well.

Assuming a part of our user base is using Atom as a text editor rather than a code editor, maybe some info regarding the Snippets can be put in the readme file. In particular the 'a' prefix, and getting the total list of snippets, and maybe the option to cycle through the fields using 'tab'.

mojavelinux commented 8 years ago

First, I really like the tab navigation to "fill in the blanks" of the structure. That's really nice and works really well.

I don't think we should assume a source block for the listing block. There are plenty of times when you want to create a listing block, but not a source block. I think we need another syntax entry for source block. I'm not sure what, though. Maybe --s?

We're missing the open block. The open block should be mapped to two tildes (~~) (since we're planning to make four or more tildes the open block in the future).

The headings are off by one. A level-0 is =. A level-1 is ==. And so forth. However, I would be okay with keeping it the way it is now and just dropping the =0. That way, you specify the number of equal signs, not the level number (which is always one less).

The macros and others are going to be a problem because they popup everywhere. The "a" prefix doesn't help here because the completion looks for any letter in the word. This is a major problem with the autocomplete package in Atom. It's super, super annoying when the prefix is a word.

What does work is if you change the prefix to a non-letter, such as hyphen. Then you can type -menu<TAB> and you'll get the menu macro. I think this is the solution until the autocomplete package gets less insane.

Action item: change the "a" prefix to "-" (or something else like $).

mojavelinux commented 8 years ago

I'd change "-callout" to just "-co".

mojavelinux commented 8 years ago

Could you remove the space before the language in the source block? Generally, I like to have no spaces between the attributes. One less character to enter.

mojavelinux commented 8 years ago

I think we could use ,, for the listing block. That doesn't seem to have the same problem as the double period. Perhaps a bit nicer than /...

mojavelinux commented 8 years ago

I'm not a fan of the autocomplete for the table. width and options are rarely needed. Usually, the only thing you need is cols. I'd stick with cols as the only attribute to complete, though you could make the case even cols aren't necessary a lot of the time. I wouldn't force a caption (aka block title) either.

mojavelinux commented 8 years ago

snippets with prefix starts with a punctuation characters: no popup but works

I actually which it worked this way by default. While the popup is useful for learning, I find the tab completion to be preferable to make it work. Then, it says out of your face.

mojavelinux commented 8 years ago

Alt+Shift+s is a great way to learn about what completions are available. We should definitely mention that in the README.

mojavelinux commented 8 years ago

Don't forget that Ctrl+/ adds a line comment to all selected lines (or the current line if no line is selected). We somehow get that one for free (not sure how it knows).

ldez commented 8 years ago

In french we love spaces before and/or after punctuation, the main example is :. In french if it's a double punctuation (like ;, :, ?, ...) we add space before and after. :wink: It's a big fail with the GitHub emojis.

Otherwise, in code we always add space after ,. The linters and static analysis tools remind us regularly.

It's a philosophical question: space or not space ?

ldez commented 8 years ago

For the prefix, I prefer a letter to a punctuation because a letter will open the autocomplete box.

I propose z, wdyt ?

ldez commented 8 years ago

For the comment Ctrl+/ it's not really free, it's me :wink: I put the settings parameters for comments for this to works.

mojavelinux commented 8 years ago

For the comment Ctrl+/ it's not really free, it's me :wink: I put the settings parameters for comments for this to works.

Then you are awesome. :tada:

mojavelinux commented 8 years ago

For the prefix, I prefer a letter to a punctuation because a letter will open the autocomplete box.

The point is, no letter we pick gets us out of this usability problem. To put it another way, if the prefix starts with any letter, then the box is going to popup at all sorts of inappropriate times and really annoy the writer. I think when writing a word in a sentence, the popup should never appear. The only way to guarantee this is to use a non-letter at the start of the prefix.

mojavelinux commented 8 years ago

Down the line, when Atom fixes this, then I think we can reconsider. Until then, I think we have to enable "silent" mode.

ldez commented 8 years ago

It's possible to quiet autocomplete box only for AsciiDoc:

atom-autocomplete-plus

ldez commented 8 years ago

I give up the support of the auto-complete box.

I propose to use / instead of - because:

And I have reduced the keyword length.

wdyt ?

mojavelinux commented 8 years ago

It's possible to quiet autocomplete box only for AsciiDoc:

Nice! Now that I know where to look for the settings, I find a number of interesting options that allow us to move forward!

I don't think we have to worry about adding a prefix anymore.

mojavelinux commented 8 years ago

I have a few suggestions coming your way.

mojavelinux commented 8 years ago

We definitely need to document how to tune the autocomplete in the README if we drop the / prefix.

mojavelinux commented 8 years ago

See https://github.com/ldez/atom-language-asciidoc/pull/1 for suggestions.

mojavelinux commented 8 years ago

And I have reduced the keyword length.

I didn't like this change. It felt unnatural. I think we should prefer the shortest full word we can find. One exception is "img" for inline image (reserving "image" for a block image).

mojavelinux commented 8 years ago

I'm still torn whether to include the / at the beginning of any prefix that is a word. As I said, now that I know how to stop the suggestions from popping up, I'm okay with not using any tricks.

It also puts back on the table the idea of using b the beginning of prefixes for blocks. Personally, I kind of like the two-character prefixes that match the delimiter characters (e.g., ==).

mojavelinux commented 8 years ago

To get more accurate completions, it would be interesting to tap into the Provider API. That would prevent completion of a block when the cursor is not at the left margin, perhaps.

ldez commented 8 years ago

To get more accurate completions, it would be interesting to tap into the Provider API. That would prevent completion of a block when the cursor is not at the left margin, perhaps.

I works on change in https://github.com/asciidoctor/atom-asciidoc-preview but it seems dead ?

mojavelinux commented 8 years ago

https://github.com/asciidoctor/atom-asciidoc-preview seems dead ?

Not dead. Just waiting for the a) release of Asciidoctor.js 1.5.4 and b) the focus once we get the language grammar stabilized.

mojavelinux commented 8 years ago

I think we're pretty close here. @nicorikken, give it a run around the block and see how you like it. Of course, we can always refine :)

nicorikken commented 8 years ago

Great work! 👍

My remarks:

  1. --s triggers stem rather than a source block.
  2. This seems to be the case for all macro's, where I have to leave out the initial / for it to register as a snippet.
  3. The revision /rev doesn't cycle through the version number, defaulting to 1.0
  4. Icon snippet doesn't work or isn't available any longer.
  5. Image include doesn't work is isn't available any longer.

Am I doing something wrong? I disabled the asciidoc-preview package to prevent it from interfering.

ldez commented 8 years ago

You speak of old version ? / chars have been removed.

No need to disabled anything.

ldez commented 8 years ago
  1. you're right (side effect)
  2. ??
  3. rev cycle fixed
  4. restore 'icon macro'
  5. Image include -> Image block
mojavelinux commented 8 years ago

In the case of icon, the first positional attribute is the size, not the alt text. I think for icon, we should only use one placholder, which is the icon name.

body: 'icon:${1:name}[] $0'

Example:

icon:heart[]
mojavelinux commented 8 years ago

--s triggers stem rather than a source block.

That does seem to be a problem if don't time it just right. Hmm, back to the drawing board.

mojavelinux commented 8 years ago

How about dollar sign?

--$
mojavelinux commented 8 years ago

Here's another suggestion:

Kind of a different approach, I'll admit. Food for thought.

mojavelinux commented 8 years ago

(we could leave the open block alone)

mojavelinux commented 8 years ago

I think the author snippet should end by advancing you to the next line, or the rev snippet should keep you on the same line. Either way, they need to be parallel.

mojavelinux commented 8 years ago

How about a snippet for an attribute entry?

:${1:name}: ${2:value}

The prefix can be double colon.

ldez commented 8 years ago

'author' can be add several times in a same line, but 'rev' only one time. It's not the same behavior.

= The Dangerous and Thrilling Documentation Chronicles
Kismet Rainbow Chameleon <kismet@asciidoctor.org>; Lazarus het_Draeke <lazarus@asciidoctor.org>
mojavelinux commented 8 years ago

Ah, of course. We could do one of the following (or a combination).

  1. Add another template for "authors" (or authors-2, authors-3, etc) that will step you through multiple authors
  2. Change rev to a single line just so that it is consistent with "author"
  3. Add a template for "info" that steps through author and revision line.

wdyt?

That doesn't have to hold up the PR. I'm just looking for ways to really make this feel nice in the hand.

nicorikken commented 8 years ago

For reference: my review comments were based on the main posts of this PR, giving the guide on the snippets use.

ldez commented 8 years ago

It's a first version. keep simple.

Others remarks ?

nicorikken commented 8 years ago

Tested and I see the improvements. I don't use snippets enough to really fuzz about the preceding characters. Fine for me 👍

mojavelinux commented 8 years ago

It's definitely time to get it into the hands of users and get some feedback. My last request is to add a snippet for an attribute entry. That's a very common case.

  'Attribute Entry':
    description: 'Define or update a document attribute'
    descriptionMoreURL: 'http://asciidoctor.org/docs/user-manual/#setting-attributes-on-a-document'
    prefix: '::'
    body: '''
      :${1:name}:${2: value}
      $0
      '''
ldez commented 8 years ago

@mojavelinux Done!

mojavelinux commented 8 years ago

Excellent! Let's proceed!

mojavelinux commented 8 years ago

:+1:

ldez commented 8 years ago

@nicorikken @mojavelinux anyone can release ? Its 1.2.0 time!

mojavelinux commented 8 years ago

It's up to you and @nicorikken to decide when to release. I usually go by the rule that at least two maintainers say "Go". When @nicorikken gives the :+1: then shoot!