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.

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

Migrate to nimib 0.3 - BlockMaker #5

Merged
merged 52 commits into from
Jul 12, 2022
Merged

Migrate to nimib 0.3 - BlockMaker #5

merged 52 commits into from
Jul 12, 2022

Conversation

HugoGranstrom
Copy link
Owner

@HugoGranstrom HugoGranstrom commented Mar 9, 2022

The hardest parts are migrated and are less hacky now. fragmentCore, fragmentStart and fragmentEnd are still a bit hacky but I think this is necessary for it to work.
There are still a lot of nbText: *html code* everywhere but they aren't important right now as they work fine. Also has some problems with nim-mustache not liking HTML and turning < into &lt; for example. Will have to look into if we can escape the tags somehow. nbText goes through the markdown processor which must somehow escape it, because it works. Found it! I have seen you prefixing fields with & in the templates but didn't know what they did. Seems like it's escaping the text :D

Overall it was a quite smooth transition and implementing animateCode was a breeze by reusing the functionality of nbCode :D

@pietroppeter Would appreciate your eyes on this when you have the time. Especially if I made something more convoluted than it had to be done :D

TODO:

  • One slide to rule them all (remove slideRight, slideLeft)
  • Update examples
  • Update README
  • Skip the initReveal call entirely and just dump everything in global?

Copy link
Contributor

@pietroppeter pietroppeter left a comment

Choose a reason for hiding this comment

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

Hi! all in all it is very nice. The code reads very well! I left just some minor remarks in the code.

Other general remarks:

  • I like the new api, very simple only adding slide
  • I like the refactoring of all mustache templates
  • I like the nbRawBlock, could steal it for nimib (I might want to rename it to nbRawHtml; or is nbHtmlRaw better? I loose so much time about these details...). not sure in that case if you want to do a PR to nimib and remove it from here. I would merge that pretty quick.

docs/index_white.nim Outdated Show resolved Hide resolved
nimiSlides.nim Outdated Show resolved Hide resolved
nimiSlides.nim Outdated Show resolved Hide resolved
nimiSlides.nim Outdated

template bigText(text: string) =
nbText: "<h2 class=\"r-fit-text\">" & text & "</h2>"
rawBlock: "<h2 class=\"r-fit-text\">" & text & "</h2>"

template removeCodeOutput =
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking, well I could add this to nimib too (checking if "Code" is in blk.command). Then I realize that it should be equivalent to a nb.blk.output = "" and if we really want we could have that kind of template in nimib (nbRemoveOutput) just for ergonomics...

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes indeed, just haven't thought of any other case than code blocks where we want to remove the output. But I could add it as well to the nbRaw* PR if you'd like :)

nimiSlides.nim Outdated Show resolved Hide resolved
@pietroppeter
Copy link
Contributor

Found it! I have seen you prefixing fields with & in the templates but didn't know what they did. Seems like it's escaping the text :D

yep indeed that is the case. In mustache you can use & or a triple mustache to escape the content. I started at some point using exclusively the & since I think it make it more easy to spot and it is a very important detail (might need to highlight it somewhere in nimib docs)

@pietroppeter
Copy link
Contributor

Skip the initReveal call entirely and just dump everything in global? Must keep this adding {.experimental: "flexibleOptionalParams".} to the user file

just saw this and I have a review comment that was suggesting to dump everything in global (since with last PR I did that for nimib). Does this mean that current devel for nimib will not work for either 1.4 or 1.6 or that it has some kind of issues?

@pietroppeter
Copy link
Contributor

the {.inject.} for the variable shouldn't be needed

I was trying to understand this comment to learn something more about templates but I was not able to find the correct commit that makes it clear. Also rather weirdly, comments in this conversation start duplicating themselves at some point...

@HugoGranstrom
Copy link
Owner Author

I was trying to understand this comment to learn something more about templates but I was not able to find the correct commit that makes it clear. Also rather weirdly, comments in this conversation start duplicating themselves at some point...

Haha yes, it is really messy 😅 I had to change it multiple times because I hadn't taken edge cases into consideration so there isn't a single commit 🙃 I don't understand it either tbh, all I know is that it doesn't work without it 🤷

@pietroppeter
Copy link
Contributor

With this implementation the typewriter will compile more or less the same Karax code for every fragment?

Wouldn't be better to have some JavaScript (added with a nb.useTypewriter) that will do that for every paragraphs with (for example) a typewriter class?

@HugoGranstrom
Copy link
Owner Author

HugoGranstrom commented Jul 2, 2022

With this implementation the typewriter will compile more or less the same Karax code for every fragment?

Indeed. That's the point of a reusable widget ;)

Wouldn't be better to have some JavaScript (added with a nb.useTypewriter) that will do that for every paragraphs with (for example) a typewriter class?

I see what you mean, but I don't see how that would simplify things really. I prefer atomic widgets over using shared code that must interact between widgets. Also then we would need to use the nbCodeToJsInit + addCodeToJs API which also would require us to insert a addToDocAsJs at the end requiring us to add a nimiSlidesEnd template or something like that that the user must remember to run before running nbSave. Or am I complicating things again?

I don't really care how the produced javascript code looks like tbh, all I want is that it is working. Then the code can be however ugly and repeating as it wants

@HugoGranstrom
Copy link
Owner Author

HugoGranstrom commented Jul 2, 2022

Ok, I re-read your message and you propose something like this?

<p class="typewriter" id="uniqueId">Type this text</p>

And then the javascript looks for all <p> tags with the typewriter class and associates each one with an object holding its id and fragmentIndex. Maybe could work but I wouldn't hide it behind a nb.useTypewriter, there's no point in doing that. I guess I'd have to if I want to insert it using nbCodeToJs 🤔

@pietroppeter
Copy link
Contributor

Well the nb.useTypeWriter would be to add a block that contains the code to convert to javascript, not to be used for the single class instance.

@HugoGranstrom
Copy link
Owner Author

OK I understand what you mean then, it would require a few {.exportjs.} and {.importjs} to make it work. But I'm against the general use of the useSomething API. The user shouldn't have to remember to call a template to setup a widget just to use the widget if it is possible to avoid. Otherwise, we might end up with code like this:

useWidget1()
useWidget2()
useWidget3()
useWidget4()
useWidget5()

just to use a couple of widgets. For the user, it only brings more complexity. Do you understand my reasoning?

@pietroppeter
Copy link
Contributor

Yes, I see, although html files being very big because they have a lot of JavaScript inside is also a issue. Anyway better to start having the widgets and after that improving on these details. :)

@HugoGranstrom
Copy link
Owner Author

HugoGranstrom commented Jul 2, 2022

Yes, I see, although html files being very big because they have a lot of JavaScript inside is also a issue.

That's a good point that the file size may grow a bit if we have to repeat the "boilerplate" code compiler creates each time. But for karax components that is a necessary evil as they have to be compiled in separate files. In this case karax is overkill of course and it could in theory be merged with other codes. It's also worth investigating minimizing the javascript code

@HugoGranstrom
Copy link
Owner Author

Adding a call to closure-compiler if it is available should be trivial to implement in nimib for example.

@HugoGranstrom
Copy link
Owner Author

I tried it and totally raw the code size is 200kb for one typewriter. Gzipped it was 33KB (and this is what we should look at as pages often are gzipped nowadays). Compressed and gzipped is was 24KB so not a huge difference, but it means we could fit 40 karax components in 1MB roughly. So unless you are going haywire with karax components we should be pretty safe.

@HugoGranstrom
Copy link
Owner Author

That's it for now, time to do a release!

I will have to add CI in a PR soon though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants