Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Added more formatting options for snippets #194

Merged
merged 4 commits into from
Mar 24, 2016
Merged

Conversation

PierBover
Copy link
Contributor

When working on a snippet I was wondering how other packages formatted the results in the autocomplete list. So I found the autocomplete-plus API can accept more parameters for formatting the results.

leftLabel (optional): This is shown before the suggestion. Useful for return values.
leftLabelHTML (optional): Use this instead of leftLabel if you want to use html for the left label.
rightLabel (optional): An indicator (e.g. function, variable) denoting the "kind" of suggestion this represents
rightLabelHTML (optional): Use this instead of rightLabel if you want to use html for the right label.

I thought it would be awesome to be able to visually format results for snippets too, so I have added those properties in the Snippet class so that you could add those in the cson files that define snippets.

The idea is to read those optional properties from the cson file and provide them to autocomplete-snippets which in turn will deliver those results to autocomplete-plus.

Additions to the Snippet class (All parameters are optional)

  • rightLabelHTML
  • leftLabel
  • leftLabelHTML

I haven't added rightLabel because autocomplete-snippets uses the name of the snippet to fill that and is what all current snippets use to display some info.

I have also worked on autocomplete-snippets locally, but I will wait for this pull request to be accepted before submitting it there.

Added rightLabelHTML, leftLabel, leftLabelHTML in the Snippet class to
be able to pass use values in a snippet, and those values to
snippets-autocomplete and finally to autocomplete-plus
@lee-dohm
Copy link
Contributor

lee-dohm commented Mar 8, 2016

To be accepted, this extra functionality would need specs and updates to the documentation in the README.

@PierBover
Copy link
Contributor Author

Thanks for the suggestions @lee-dohm, I've added a description of the fork in my first comment and updated the readme.

@PierBover
Copy link
Contributor Author

Is there anything else I should do?

@50Wliu
Copy link
Contributor

50Wliu commented Mar 13, 2016

@PierBover The new functionality still needs specs to make sure it doesn't regress in the future.

@PierBover
Copy link
Contributor Author

Thanks @50Wliu

Could you point me to an example of the type of specs you are expecting?

@50Wliu
Copy link
Contributor

50Wliu commented Mar 13, 2016

Hmm, it looks like there aren't any tests for the existing parameters either 😦. Since all you're doing here is passing some new params along and not actually doing anything with them, I think it'd actually be fine not to add specs here and instead add them in autocomplete-snippets or autocomplete-plus. Thoughts @lee-dohm?


* `leftLabel` will add text to the left part of the autocomplete results box.
* `leftLabelHTML` will overwrite what's in `leftLabel` and allow you to use a bit of CSS such as `color`.
* `rightLabelHTML`. By default, in th right part of the results box you will see the name of the snippet. When using `rightLabelHTML` the name of the snippet will no longer be displayed, and you will be able to use a bit of CSS.
Copy link
Contributor

Choose a reason for hiding this comment

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

By default, in *the* right part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it

@PierBover
Copy link
Contributor Author

I've also modified autocomplete-snippets to receive those parameters and pass them to autocomplete-plus. Should I add a PR there or wait for this PR to be implemented here first?

@lee-dohm
Copy link
Contributor

I'd expect to at least see something like this that shows the new snippet values get parsed correctly from a sample snippet file:

https://github.com/atom/snippets/blob/master/spec/snippet-loading-spec.coffee#L33-L49

@PierBover
Copy link
Contributor Author

@lee-dohm it seems the part you highlighted is for required arguments, although since this is the first time I'm writing specs for Atom and writing in CS I'm not so sure.

I've updated test.cson with 2 more cases using these new optional properties, and snippet-loading-spec.coffee too to test for that (I think).

Checks have passed... let me know if there's anything else I must do.

@PierBover
Copy link
Contributor Author

Hey @lee-dohm and @50Wliu

The modifications I made to the snippets class and specs are identical (obviously with different parameters) to these ones added last year.

#132

And that PR was accepted...

Is there any reason why you won't accept my PR?
What else do you want me to do?

@lee-dohm
Copy link
Contributor

Thanks for the ping @PierBover. I missed your earlier post. Unfortunately with a project this size, sometimes that happens 😞

On the face of it, this looks good. I need to do some more testing and then if everything is working, this should be good to go.

@PierBover
Copy link
Contributor Author

Awesome!

@lee-dohm are you also in charge of autocomplete-snippets ?

@lee-dohm
Copy link
Contributor

I don't know that I'm necessarily "in charge" of anything 😀 But I've got my fingers in everything 😆 Why do you ask?

@PierBover
Copy link
Contributor Author

:)

Sorry... I'm a bit of a noob at contributing to Atom.

I was asking because if / when this PR gets accepted I will need to make a PR in autocomplete-snippets to be able to get the full functionality. I don't know if I should wait to do that or not.

@lee-dohm
Copy link
Contributor

You can create it now and just say in the PR comment that it is dependent
on this one.

On Fri, Mar 18, 2016 at 7:13 PM, Pier Bover notifications@github.com
wrote:

:)

Sorry... I'm a bit of a noob at contributing to Atom.

I was asking because if / when this PR gets accepted I will need to make a
PR in autocomplete-snippets to be able to get the full functionality. I
don't know if I should wait to do that or not.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#194 (comment)

@lee-dohm lee-dohm merged commit 5a15bef into atom:master Mar 24, 2016
@lee-dohm
Copy link
Contributor

Thanks for contributing! Sorry for the delay 😀

@PierBover
Copy link
Contributor Author

Yay! :)

I've been super busy but I will create the PR on autocomplete-snippets these days.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants