fix(convert): Whitespace issue in default RevealJS template

yunusey commented 1 month ago

Description

First of all, thanks for the amazing project!

Recently, I was trying to deploy my animation to web using RevealJS where I wanted the first slide to loop. On the player, everything was working fine, but on the website, the first slide wasn't looping. When I looked at the produced html file, I saw this:

...
  <body>
    <div class="reveal">
      <div class="slides"><section
              data-background-size='contain'
              data-background-color="#181926"
              data-background-video="index_assets/{very_long_asset_id}.mp4"
              data-background-video-muteddata-background-video-loop>

            </section></div>
    </div>

    <script src="https://cdnjs.cloudflare.com/ajax/libs/reveal.js/5.1.0/reveal.min.js"></script>
...

I think there's a problem with the spacing of data-background-video-muted and data-background-video-loop default RevealJS template.

I tested my current change in my slides, it worked fine. I recommend checking out Jinja2's whitespace control.

Check List (Check all the applicable boxes)

[x] I understand that my contributions needs to pass the checks.
[x] If I created new functions / methods, I documented them and add type hints.
[x] If I modified already existing code, I updated the documentation accordingly.
[x] The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

jeertmans commented 1 month ago

Hello @yunusey, thanks for catching this bug!

I recognize that the "whitespace trimming" performed by Jinja2 is not very consistent in my template, and I definitely should improve it!

Would you mind adding an entry to the CHANGELOG file?

jeertmans commented 1 month ago

And thanks for linking to the appropriate documentation, I didn't know about the trim_blocks options!

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.30%. Comparing base (c0a240d) to head (c27e112).

:exclamation: Current head c27e112 differs from pull request most recent head ddc8394

Please upload reports for the commit ddc8394 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #442 +/- ## ======================================= Coverage 79.30% 79.30% ======================================= Files 22 22 Lines 1822 1822 ======================================= Hits 1445 1445 Misses 377 377 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yunusey commented 1 month ago

Hi @jeertmans!

I've added the entry (ddc8394) - I hope you should be able to see it now. I just checked #443, everything looks great. Please let me know if there's anything else I can help with!

jeertmans / manim-slides