Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(convert): Whitespace issue in default RevealJS template #442

Merged
merged 2 commits into from
May 27, 2024
Merged

fix(convert): Whitespace issue in default RevealJS template #442

merged 2 commits into from
May 27, 2024

Conversation

yunusey
Copy link
Contributor

@yunusey yunusey commented May 26, 2024 •
edited
Loading

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)

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

Screenshots

Note to reviewers

@jeertmans
Copy link
Owner

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 jeertmans added bug Something isn't working html-convert Related to converting to HTML slides labels May 27, 2024
@jeertmans jeertmans modified the milestone: v6 May 27, 2024
@jeertmans
Copy link
Owner

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

@codecov Codecov
Copy link

codecov bot commented May 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

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

❗ 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 
@@           Coverage Diff           @@
##             main     #442   +/-   ##
=======================================
  Coverage   79.30%   79.30%           
=======================================
  Files          22       22           
  Lines        1822     1822           
=======================================
  Hits         1445     1445           
  Misses        377      377

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jeertmans added a commit that referenced this pull request May 27, 2024
@jeertmans
chore(convert): improve HTML Jinja2 template
27cf105 
As suggested by @yunusey in #442
@jeertmans jeertmans mentioned this pull request May 27, 2024
Open
3 tasks
@yunusey
Copy link
Contributor Author

yunusey commented May 27, 2024

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
jeertmans approved these changes May 27, 2024
View reviewed changes
Copy link
Owner

@jeertmans jeertmans left a comment

Choose a reason for hiding this comment

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

Thank you, that’s great!

@jeertmans jeertmans merged commit 9460f6b into jeertmans:main May 27, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeertmans jeertmans jeertmans approved these changes

Assignees
No one assigned
Labels
bug Something isn't working html-convert Related to converting to HTML slides
Projects
None yet
Milestone
v6
Development

Successfully merging this pull request may close these issues.
2 participants
@yunusey @jeertmans