-
Notifications
You must be signed in to change notification settings - Fork 52
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
Composable helpers #62
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going the right direction, thanks.
I'm fine with moving the snippets themselves into treeForAddon
, I agree it's a better place for them.
I agree that we don't really need a code-snippet
component and may want to leave that name free to be used by the other addon that implements syntax highlighting.
Thanks @ef4 for the review! I pushed the following update:
The only remaining question is how we should handle the actual feature that kicked off all of this: the dynamic replacement...
Again, I don't really have a good use case for having that replacement helper without this addon, and as the code is pretty simple, it feels a bit as an unnecessary overhead to extract that into its own addon. But I am fine with a decision either way... |
I removed this for now, to unblock this. We can later add it back, either here in a separate PR, or as another package... @ef4 As there were quite a few changes after your initial review, and as this changes the public API radically, I would love your final approval before merging this! 🙏 |
README.md
Outdated
includeHighlightStyle: false | ||
}); | ||
```hbs | ||
{{#code-block language="handlebars"}}{{get (get-code-snippet "demo.hbs") "source"}}{{/code-block}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use an example like this?
{{#let (get-code-snippet "demo.hbs") as |snippet|}}
{{#code-block language=snippet.language}}
{{snippet.source]}
{{/code-block}}
{{/let}}
It would show why we decided to return a POJO with the language too. This snippet can more easily generalize to a reusable component that accepts the snippet name as an argument:
{{!-- templates/components/code-snippet.hbs --}}
{{#let (get-code-snippet @name) as |snippet|}}
{{#code-block language=snippet.language}}
{{snippet.source]}
{{/code-block}}
{{/let}}
Which you would use like:
<CodeSnippet @name="demo.hbs" />
Maybe we should even suggest that pattern in the README. I think most apps that will have many snippets should make a component like this that puts together their chosen syntax highlighter and get-code-snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and also we should explain this is the upgrade path for people who were using the current version of the addon. If they just want to keep the behavior they had, they should make their own <CodeSnippet />
component to replace the one we removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated according to your suggestion!
It would show why we decided to return a POJO with the language too
There is one potential problem though: that example would actually not work properly with prism.js and hbs templates, as our helper returns 'htmlbars'
as the language, while prism expects 'handlebars'
. But as that the language value is not standardized across highlighters, there is not so much we can do about it!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If prism is going to be our recommended syntax highlighter, I'm fine with changing our helper to return handlebars
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized I committed my recent Readme updates to the wrong branch. Fixed! 🤦♂️
If prism is going to be our recommended syntax highlighter, I'm fine with changing our helper to return handlebars.
Ok, agree. Updated that, including a test + docs.
Published as 3.0.0. |
@simonihmig Are you still planning to PR this change? |
@jrowlingson no, I chose for now to implement this just for myself here: https://github.com/kaliber5/ember-bootstrap/blob/master/docs/app/helpers/dynamic-code-snippet.js. But feel free to reuse it, if this is what you are looking for! |
This is a spike at implementing some of the ideas of #56 (comment) and #61 (comment):
hightlight.js
code as it de-scopes the concern of code highlightingget-code-snippet
helper exposing a{ source, language, extension }
POJOaddon/index.js
for use cases where those are needed in JS land rather than only in templatescode-snippet
component (again without any highlighting) for straightforward rendering, but now implemented based on the helper. This could be removed, as it is just for convenience.{{...}}
instances), so I think the use case is pretty much coupled to the functionality ofember-code-snippet
... 🤔ember-prism
ember-code-snippet/snippets
instead of<app-name>/snippets
(merged intotreeForAddon
now), to make it easily importable from the static JS modules (i.e. not knowing the app's name). AFAICT that module should be considered private, although it has been used in the wild: https://github.com/ember-learn/ember-cli-addon-docs/blob/master/addon/components/docs-snippet/component.js#L4. But these are major breaking changes anyway, so should be ok!?To Do: