google / comprehensive-rust

This is the Rust course used by the Android team at Google. It provides you the material to quickly teach Rust.
https://google.github.io/comprehensive-rust/
Apache License 2.0
27.45k stars 1.63k forks source link

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

Open michael-kerscher opened 1 month ago

michael-kerscher commented 1 month ago

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 commented 1 month ago

Thanks Michael, that would be a nice tool indeed!

michael-kerscher commented 3 weeks ago

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 commented 1 week ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 https://github.com/google/comprehensive-rust/commit/d5b92dbb5f90057c94015a02eb7cfc7026f8cdea 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 https://github.com/michael-kerscher/comprehensive-rust/commit/c5cb6399dabb142830f83804fe721a446f54ab0c)

@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 commented 3 days ago

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