added fitVideo()

[hlclemson]

No description provided.

[hlclemson]

I apologize in advance if this is too basic of a question. Are the checks on both the stable and devel versions failing due to the commit I made?

I don't quite understand why it's indicating a type mismatch on this line:

slide:
  for n in [50, 100, 1000, 10000]:
    let filename = &"images/gauss-{n}.png"
    let samples = newSeqWith[float](n, gauss(0.0, 1.0))
    let df = toDf(samples)
    ggplot(df, aes("samples")) +
      geom_histogram(fillColor="green") +
      ggsave(filename)

[HugoGranstrom]

First of all, thanks for contributing 😁

Secondly, no it isn't your fault. I think it is a change in the Nim stdlib for the newSeqWith function changing that breaks the CI. If I understand it correctly, it isn't generic anymore so we should write newSeqWith(...) instead if newSeqWith[float](...). Would you mind making that change in this PR? 😃 Otherwise I'll investigate myself tomorrow. I'll give your PR a try tomorrow either way and if everything works I'll merge it tomorrow night 🚀

[hlclemson]

I updated my pull request based on your feedback! Thank you so much.

[HugoGranstrom]

The CI works again! 🥳 Thank you, I'll give your code a try tonight 😄

[HugoGranstrom]

Ahh, I baked some apple cakes and forgot about you 😅 I'll give it a try tomorrow instead

[hlclemson]

I've also added an option to control the transition effect on slideOptions(). It seems to be working as intended when I tested it.

slide(slideOptions(transition="concave")):
    slide(slideOptions(transition="concave")):
        nbText: "sometext1"
    slide(slideOptions(transition="fade")):
        nbText: "sometext2"
    slide:
        nbText: "sometext3"
slide(slideOptions(transition="zoom")):
    nbText: "sometext4"

I'm not sure if it was a good idea to add it on this pull request.

[HugoGranstrom]

Wow, even more features! :D What would be really nice with the transition is if we could make an enum with the supported transitions and pass them in instead of raw strings. Like we do for fragments:

nimiSlides/src/nimiSlides.nim

Line 11 in 2fe737b

FragmentAnimation* = enum

Would it be too much of an ask?

I will try out the fitVideo right now though 😄

[HugoGranstrom]

fitVideo works like a charm 🚀

[hlclemson]

Wow, even more features! :D What would be really nice with the transition is if we could make an enum with the supported transitions and pass them in instead of raw strings. Like we do for fragments:

nimiSlides/src/nimiSlides.nim

Line 11 in 2fe737b

FragmentAnimation* = enum

Would it be too much of an ask?
I will try out the fitVideo right now though 😄

Thank you for the feedback! I will try this idea as soon as I have some free time.

[HugoGranstrom]

Thanks, no hurry. Do it when you have the time and energy 😁

[hlclemson]

Could you give me some advice on the transition implementation? I am pretty new to programming, so I don't have a good grasp on JavaScript

This is what I tried to do in the wrapper

type
  SlideTransition* = enum
    slide = "slide" # default
    none = "none"
    fade = "fade"
    convex = "convex"
    concave = "concave"
    zoom = "zoom"

type
  SlideTransitionSpeed* = enum
    fast = "fast" # default
    slow = "slow"

template slide*(transition: SlideTransition, body: untyped) =
  if transition.len > 1:
    transition = fmt"{transition[0]}-in {transition[1]}-out"
  else:
    transition = transitions.join(" ")
  var options = """data-transition="$1" """ % [transition]
  nbRawHtml: "<section $1>" % [options]
  when declaredInScope(CountVarNimiSlide):
    when CountVarNimiSlide < 2:
      static: inc CountVarNimiSlide
      body
      static: dec CountVarNimiSlide
    else:
      {.error: "You can only nest slides once!".}
  else:
    var CountVarNimiSlide {.inject, compileTime.} = 1 # we just entered the first level
    body
    static: dec CountVarNimiSlide
  nbRawHtml: "</section>"

And, this is what I put in my test code. I am getting compiler error messages that have something to do with nimib and nb.blk.

slide(zoom):
    nbText: "sometext3"

[HugoGranstrom]

I can absolutely help out :D

The enum looks great! The compiler is a bit picky when it comes to overloading untyped parameters (the body in the case of slide). So we want to use the slideOptions as you have done in this PR already. But instead of:

proc slideOptions*(transition = "slide", ....)

You do

proc slideOptions*(transition = SlideTransition.slide, ....)

And where you previously used transition when creating the HTML, you now use $transition to get the string value of the enum.

This is just a way of ensuring we are only inputting valid string by forcing the user to pass in an enum instead.

Hope this clarified some parts, otherwise you can just ask again :D

[HugoGranstrom]

if transition.len > 1:
    transition = fmt"{transition[0]}-in {transition[1]}-out"
else:
  transition = transitions.join(" ")

Is transition supposed to be an array of SlideTransition or just a single SlideTransition? The template argument suggests the latter but the code indicates the former.

[hlclemson]

if transition.len > 1:
    transition = fmt"{transition[0]}-in {transition[1]}-out"
else:
  transition = transitions.join(" ")

Is transition supposed to be an array of SlideTransition or just a single SlideTransition? The template argument suggests the latter but the code indicates the former.

According to the Reveal.js documentation, it appears that you can control the in and out transitions separately, and even adjust the transition speed. I think there should be an option to pass an array (or sequence) to support all these features.

So, I've made a preliminary attempt to parse the passed array as a string. But, I'm not entirely sure about the best design choice to support these features.

<section data-transition="zoom">
 <section data-transition="fade-in slide-out">
<section data-transition-speed="fast">

[HugoGranstrom]

Could you make each of them separate input argument perhaps?

slideOption(transition=slide, outTransition=zoom, transitionSpeed=fast)

And if you set the default value of outTransition=transition (that's a cool feature of Nim) you would just have to set transition to set both of them. So the proc signature would be:

proc slideOptions*(autoAnimate = false, iframeInteractive = true, colorBackground, imageBackground, videoBackground, iframeBackground, gradientBackground: string = "", transition = SlideTransition.slide, outTransition = transition, transitionSpeed = SlideTransitionSpeed.fast): SlideOptions =

Does that make sense?

[hlclemson]

Thank you for the feedback. I implemented the feature based on your feedback and it's mostly working.

Here is my test code

slide(slideOptions(transition=convex, transitionSpeed=slow)):
    nbText: "sometext1"

slide(slideOptions(transition=none, transitionSpeed=slow)):
    nbText: "sometext2"

slide(slideOptions(transition=fade, transitionSpeed=slow)):
    nbText: "sometext3"

slide(slideOptions(transition=zoom, transitionSpeed=slow)):
    nbText: "sometext4"

slide(slideOptions(transition=convex, transitionSpeed=slow)):
    nbText: "sometext5"

slide(slideOptions(transition=convex, transitionSpeed=fast)):
    nbText: "sometext6"

slide(slideOptions(transition=convex, transitionSpeed=fast)):
    nbText: "sometext7"

slide(slideOptions(transition=convex, outTransition=zoom, transitionSpeed=fast)):
    nbText: "sometext8"

slide(slideOptions(transition=convex, outTransition=zoom, transitionSpeed=slow)):
    nbText: "sometext9"

slide(slideOptions(transition=none, outTransition=none, transitionSpeed=slow)):
    nbText: "sometext10"

However, it seems that the compiler gives me a type mismatch error if I pass the option "slide" directly to the transition. I am not sure how I should handle this.

slide(slideOptions(transition=slide, outTransition=none, transitionSpeed=slow)):
    nbText: "sometext10"

Error: type mismatch
Expression: slideOptions(transition = slide, outTransition = none, transitionSpeed = slow)
  [1] transition = slide: transition: None
  [2] outTransition = none: outTransition: SlideTransition
  [3] transitionSpeed = slow: transitionSpeed: SlideTransitionSpeed

Expected one of (first mismatch at [position]):
[1] proc slideOptions(autoAnimate = false; iframeInteractive = true;
    colorBackground, imageBackground, videoBackground, iframeBackground,
    gradientBackground: string = ""; transition = SlideTransition.slide;
                  outTransition = transition;
                  transitionSpeed = SlideTransitionSpeed.fast): SlideOptions

[HugoGranstrom]

Awesome! 😄
I suspect it might be caused by the compiler being confused because the slide template is also being called slide. Does it work if you write the full name SlideTransition.slide?

[hlclemson]

Yep, it seems like passing the full name compiles the code

slide(slideOptions(transition=SlideTransition.slide, outTransition=none, transitionSpeed=slow)):

[HugoGranstrom]

One solution would be to add a prefix to the transition. For example tslide (for transition-slide) so that we don't get any conflicts. As it is the default value, it will probably be the least used one and then it can be a bit ugly without affecting too much. The other ones can keep their current names as they don't cause any conflicts. What do you think?

[hlclemson]

One solution would be to add a prefix to the transition. For example tslide (for transition-slide) so that we don't get any conflicts. As it is the default value, it will probably be the least used one and then it can be a bit ugly without affecting too much. The other ones can keep their current names as they don't cause any conflicts. What do you think?

Hmm, at the moment, I cannot think of any clean way to handle this issue. As you said, this may not become a problem in most cases since there is no reason to pass the default transition option explicitly.

My opinion is that the current state is pretty usable, and we can fix this later if it becomes an issue, I think.

[HugoGranstrom]

I have looked thorugh the code now and there was only one question I had.

Other than that, it would be nice if these new features where documented somewhere. I understand if you are fed up with this PR by now so it's ok if you don't do it. I can do it someday instead, that works for me. But let me know if you want to write docs. And in that case it would fit best in docsrc/tutorials/images_media.nim (fitVideo) and docsrc/tutorials/slide_options.nim.

[HugoGranstrom]

Do we want the enum to be pure, i.e. that you have to write SlideTransition.convex?

[hlclemson]

My understanding of the {.pure.} pragma was the opposite. I wanted to avoid calling the full name and just use 'convex.' I tried to compile my test code without the pragma, and it wouldn't compile.

[HugoGranstrom]

Huh, that's weird 🤔

According to the manual it should be the opposite:

An enum type can be marked as pure. Then access of its fields always requires full qualification.

I'll try running your code without it and see if I can find something

[HugoGranstrom]

I am also getting an error without it so something fishy is going on. But as it seems to work, we can keep the pure pragma for now :)

[HugoGranstrom]

It doesn't even work when doing SlideTransition.convex without the pure pragma, really strange 🤯

[hlclemson]

Huh, that's weird 🤔

According to the manual it should be the opposite:

An enum type can be marked as pure. Then access of its fields always requires full qualification.

I'll try running your code without it and see if I can find something

I see, the manual is a little bit confusing to me. I tried to understand the pragma based on this example.
https://nim-by-example.github.io/types/enums/

My guess was that the {.pure.} pragma does two things.

1. It forces you to be explicit when there are overlapping names among all the declared enums.
2. It allows you to refer to enums without qualification if there is no ambiguity.

[HugoGranstrom]

Ones again, thank you so much for contributing to nimiSlides and that you have endured my comments ❤️

[hlclemson]

Ones again, thank you so much for contributing to nimiSlides and that you have endured my comments ❤️

No worries! I learned a lot from your feedback. Thank you so much again.
I will take care of the documentation part within a couple of days.

[HugoGranstrom]

No worries! I learned a lot from your feedback. Thank you so much again.
I will take care of the documentation part within a couple of days.

I'm glad to hear that, always fun helping others out 😄
Sounds good, no pressure though. Do it whenever you feel like :D