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

Automated slide size analysis with a new slide scoring tool #2234

Open
michael-kerscher opened this issue Jul 19, 2024 · 8 comments
Open

Automated slide size analysis with a new slide scoring tool #2234

michael-kerscher opened this issue Jul 19, 2024 · 8 comments

Comments

@michael-kerscher
Copy link
Collaborator

The comprehensive rust training slides vary in size and some contain too much content and are getting too big to be used in classes as scrolling starts to be necessary. Once you have to scroll a slide this throws people off during class and this should be avoided therefore.

#1464 tries to give an overview on why slides that are too big are not useful and how good slides could look like. Unfortunately this is a very manual task to look over all the slides to find violations against the the rule to not need to rely on scrolling.

I propose a small tool to create automated statistics on the size of slides, find violations against a to be defined policy and even score them to sort them to be able to focus and target on the slides that need most work.

Such a tool would enable to easily change themes or other large scale changes and see how this affects the slides.
Additionally this can be used by contributors to easily judge locally if the slide is getting too big and later on this could also be used in the CI.

The scoring tool could use selenium via webdriver with the crate fantoccini that offers methods like Element.rectangle().

@mgeisler
Copy link
Collaborator

Thanks Michael, that would be a nice tool indeed!

@michael-kerscher
Copy link
Collaborator Author

I'm not sure yet how to an integration in the workflow should look like as this is a quite expensive operation (3mins for the English translation).
Different translations don't necessarily have the same length but probably use a similar amount of pixels on the screen so it might be sufficient to only have this for the English translation in the beginning.
Later stage could be detecting what languages have been changed and only render these.

mgeisler added a commit that referenced this issue Aug 21, 2024
I created a first implementation for the mdbook-slide-evaluator I
described in #2234.

One has to run a WebDriver compatible browser (e.g. selenium-chromium)
so the slides can be rendered. The browser can access the file either
via a file:// or http:// uri.

The tool grabs the configurable element from that page and evaluates the
size of this element. Output can be stored in a csv file or at stdout
and looks like this at the moment:
```
$ mdbook-slide-evaluator book/html/android/aidl
book/html/android/aidl/birthday-service.html: 750x134
book/html/android/aidl/example-service/changing-definition.html: 750x555
book/html/android/aidl/example-service/changing-implementation.html: 750x786
book/html/android/aidl/example-service/client.html: 750x1096
book/html/android/aidl/example-service/deploy.html: 750x635
book/html/android/aidl/example-service/interface.html: 750x570
book/html/android/aidl/example-service/server.html: 750x837
book/html/android/aidl/example-service/service-bindings.html: 750x483
book/html/android/aidl/example-service/service.html: 750x711
book/html/android/aidl/types/arrays.html: 750x291
book/html/android/aidl/types/file-descriptor.html: 750x1114
book/html/android/aidl/types/objects.html: 750x1258
book/html/android/aidl/types/parcelables.html: 750x637
book/html/android/aidl/types/primitives.html: 750x366
book/html/android/aidl/types.html: 750x197
```

---------

Co-authored-by: Martin Geisler <[email protected]>
@mgeisler
Copy link
Collaborator

I'm not sure yet how to an integration in the workflow should look like as this is a quite expensive operation (3mins for the English translation).

Yeah, we should be able to just run this in another parallel job.

To start with, I guess we need to fine-tune the tool a bit to make it give us a report that we can file issues from?

We have some CSS and JS for drawing the big red rectangle you see in #1464: theme/redbox.js. I forget how we trigger this, but it should be easy enough to find.

That box has been my rule of thumb for when slides become too tall. I guess you can use that as a first step?

@michael-kerscher
Copy link
Collaborator Author

The redbox functionality seems to be included in the instructor-menu.js. But I don't see how this is called as there is no reference in other files. There is an open pull request for that though.
Am I missing something here and I should be able to show this (in non-published builds)?

@djmitche
Copy link
Collaborator

djmitche commented Sep 6, 2024

Maybe this isn't what you're asking, but

(function handleInstructor() {
  function handleInstructorMenu() {
    ..
  handleInstructorMenu();
    ..
  );
})();

So handleInstructor is an IIFE and runs immediately, and that invokes handleInstructorMenu.

@michael-kerscher
Copy link
Collaborator Author

That sounds correct, yes. But I cannot see how this would be executed. The instructor-menu.js is not loaded from any other file as far as I can see.

After some investigation in this repo I found the reason in this commit d5b92db that is disabling exactly this functionality since March.
I had to manually patch this in again and include redbox and the instructor-menu into the index.hbs template to see the menu (see michael-kerscher@c5cb639)

@djmitche @mgeisler any plans on reintegrating this back or are there any bigger issues that I'm not aware of?

It would be nice to activate this, render the red box for the violating slides so it is more easily discoverable how much the slide violates the slide policy

@djmitche
Copy link
Collaborator

djmitche commented Sep 6, 2024

We could probably re-introduce just the red box, without the UI to enable it for users. WDYT @mgeisler ?

@mgeisler
Copy link
Collaborator

@djmitche @mgeisler any plans on reintegrating this back or are there any bigger issues that I'm not aware of?

There are no plans here — I consider the red box a hack, so the less we see of it in the code and in the deployed book, the better. In particular, I was not fond of the developer menu since 99% of all people who visit the pages won't need it.

It would be nice to activate this, render the red box for the violating slides so it is more easily discoverable how much the slide violates the slide policy

Yeah, I definitely think we should have something like this on the screenshots produced by your tool.

I would probably lean towards the old mdbook plugin trick I used originally: it's crude, but it keeps the cruft contained to the little script. People can enable it locally so it works with mdbook serve and we can enable it in CI, run mdbook build, and then run the slide evaluator.

If people think it's nicer to build this functionality into the theme directly, then that is also okay. We just need a nice mechanism to enable the box.

We could probably re-introduce just the red box, without the UI to enable it for users. WDYT @mgeisler ?

I guess I'm only wondering how it would be enabled? Via a JS call in the console? That could work as well, but I think a mdbook plugin is easier to control.

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

No branches or pull requests

3 participants