orchidhq / Orchid

Build and deploy beautiful documentation sites that grow with you
https://orchid.run
GNU General Public License v3.0
513 stars 53 forks source link

New feature: Spotify embed tag for Orchid Writers Blocks #326

Closed singularsyntax closed 4 years ago

singularsyntax commented 4 years ago

Hello,

I've implemented a new tag for Orchid Writers Blocks - it allows embedding a Spotify track or playlist into a post, e.g.:

{% spotify 'track | playlist' 'trackOrPlaylistID' %}

Does this seem like a worthwhile inclusion in the Orchid Writers Blocks plugin? If so, I'll submit a PR.

Thanks.

cjbrooks12 commented 4 years ago

Absolutely, that would be great! I'll gladly accept a PR for this, thank you!

singularsyntax commented 4 years ago

I'm not familiar with Gradle ... any hints on doing a local build for testing?

cjbrooks12 commented 4 years ago

Running ./gradlew build from the repo root will do a complete build and run all tests. You can have it build/test only the Writers Blocks plugin with ./gradlew :languageExtensions:OrchidWritersBlocks:build which should be quite a bit faster.

Most of Orchid's tests are integration tests, and there is a testing framework that makes local development of plugins really easy. Refer to an example such as DiagramsTests.kt. Adding serveOn(8080) as the first line of the test will serve the site locally and can be run directly from IntelliJ (or should work from any IDE that supports JUnit5).

The following snippet should help you get started making the test. Should just need to match up the assertions.

class SpotifyTagTest : OrchidIntegrationTest(withGenerator<HomepageGenerator>(), WritersBlocksModule()) {

    @Test
    @DisplayName("Test Spotify tag as playlist embed")
    fun test01() {
        serveOn(8080)

        resource(
            "homepage.md",
            """
            |{% spotify 'track' 'playlistId' %}
            """.trimMargin()
        )

        expectThat(execute())
            .pageWasRendered("/index.html") {
                get { content }
                    .asHtml()
                    .select("body") {
                        innerHtmlMatches() {
                            iframe {
                                src = "..."
                            }
                        }
                    }
            }
    }
}
singularsyntax commented 4 years ago

Thanks, that's very helpful. But, I think something is missing, because the tag gets rendered directly into the generated page, without processing, e.g.:

{% spotify 'track' '0Vkk4vLcrUTYODEiuV9ECP' %}

I don't think it's a problem with my tag implementation, because the same happens if I replace it with one of the existing ones like {% youtube 'videoId' %}.

cjbrooks12 commented 4 years ago

Ah, my bad, it should be homepage.peb not homepage.md. The resource() follows the same rules as normal for processing files, either processed based on file extension, or it needs Front Matter to precompile as Pebble

singularsyntax commented 4 years ago

I'm still have a couple of issues. Using homepage.peb works, but I think the more common use case (and in particular, mine) would be with Markdown content, so a more specific and stronger test assertion should use homepage.md. However, I'm not sure how I can get Front Matter into the content with the resource function. I tried this, but it doesn't work:

    resource(
        "homepage.md",
        """
        |---\ntest: true\n---\n{% spotify 'track' '0Vkk4vLcrUTYODEiuV9ECP' %}
        """.trimMargin()
    )

Also, something very strange is happening with the Pebble template processing (whenever I test using homepage.peb). The HTML gets rendered as:

<div class="spotify-embed">
  <iframe src="https://open.spotify.com/embed/track0Vkk4vLcrUTYODEiuV9ECP/" allow="encrypted-media"></iframe>
</div>

The Pebble template is just this:

<div class="spotify-embed">
  <iframe src="https://open.spotify.com/embed/{{ tag.type }}/{{ tag.id }}" allow="encrypted-media"></iframe>
</div>

It's eating the / between the tag type and ID, and appending one onto the end. However, it's not that different from the GitHub Gist template:

{% if tag.file is not empty %}
<script src="https://gist.github.com/{{ tag.user }}/{{ tag.id }}.js?file={{ tag.file }}"></script>
{% else %}
<script src="https://gist.github.com/{{ tag.user }}/{{ tag.id }}.js"></script>
{% endif %}

The only substantial difference is that the Gist template encloses the HTML between some conditional {% ... %} syntax. I tried that also in my template, but it didn't make a difference.

Any ideas?

cjbrooks12 commented 4 years ago

That's strange indeed. Can you push your changes to your fork so I could pull it down and take a look?

As far as the homepage resource goes, Strings between """ in Kotlin are multiline strings. The .trimMargin() call after the string literal will trim each line until the | character, so you can set it up pretty much as you would see it in a normal file:

resource(
    "homepage.md",
    """
    |---
    |test: true
    |---
    |{% spotify 'track' '0Vkk4vLcrUTYODEiuV9ECP' %}
    """.trimMargin().trim()
)
singularsyntax commented 4 years ago

I think I see what's happening with the Pebble processing. It's concatenating both arguments into a single string "track0Vkk4vLcrUTYODEiuV9ECP", applying that to the first variable placeholder, and applying an empty string to the second.

What's the correct way to supply multiple arguments to a template tag?

singularsyntax commented 4 years ago

The above works for Markdown content. I've pushed the feature branch, including not passing integration test.

cjbrooks12 commented 4 years ago

After poking around, I believe this is a bug in the Pebble parser, not in Orchid. I'll work to get this fixed in Pebble, but until then you'll have to use named parameters. I've confirmed that {% spotify type='track' id='0Vkk4vLcrUTYODEiuV9ECP' %} works as expected in your branch.

cjbrooks12 commented 4 years ago

I've opened an issue on the Pebble repo to track this parser bug: https://github.com/PebbleTemplates/pebble/issues/481

singularsyntax commented 4 years ago

I'm not able to get the integration test working. The Kotlin HTML DSL isn't well documented, the syntax for defining multiple attributes on a tag is unclear, and I can't find any examples. I've disabled it.

What's your stance on test coverage? I can submit a PR for the basic functionality which is working, but I don't have time at the moment to look further into how the HTML DSL works.

cjbrooks12 commented 4 years ago

I'm fine without the full assertions set up; as long as the tag is running in a test and doesn't crash, that's good enough for me. Just change the assertion block to the following as a sanity check that something is happening, and I'll happily accept your PR. Thanks for your work on this!

expectThat(execute())
    .pageWasRendered("/index.html") {
        get { content }.contains("iframe")
    }