search

numberscope / frontscope

Numberscope's front end and user interface: responsible for specifying sequences and defining and displaying visualizers
MIT License
7 stars 15 forks source link

Add a template for p5 visualizers #293

Closed Vectornaut closed 7 months ago

Vectornaut commented 7 months ago

By submitting this PR, I am indicating to the Numberscope maintainers that I have read and understood the contributing guidelines and that this PR follows those guidelines to the best of my knowledge. I have also read the pull request checklist and followed the instructions therein.

This update adds a new visualizer directory, src/workbench, whose contents don't normally show up in the visualizer list. To use the visualizers in this directory, you serve Frontscope in "workbench mode" by calling npm run dev:workbench.

If this pull request looks generally okay, I'll go on to document the workbench feature.

Right now, there's only one workbench visualizer: "p5 Visualizer Template", also added in this update.

gwhitney commented 7 months ago

Ugh, I forgot that name was inherited from Paramable by Visualizer when I switched the Visualizer name instance property to the static visualizationName so it would (A) only depend on the class, and (B) be accessible without constructing an instance.

So now the usage of name is internally inconsistent in our codebase. Let's not worry about that for now. I will file an issue about this when this PR lands :(

Vectornaut commented 7 months ago

The doc http://localhost:5050/doc/doc/onboarding/ should have a link to the new visualizer docs being made here.

I've added a cursory link at the bottom, on the principle that something is better than nothing.

gwhitney commented 7 months ago

The doc http://localhost:5050/doc/doc/onboarding/ should have a link to the new visualizer docs being made here.

I've added a cursory link at the bottom, on the principle that something is better than nothing.

Looks completely adequate to me. Consider this "conversation resolved".

Vectornaut commented 7 months ago

I think I've addressed all the biggest feedback, leaving only stuff that can reasonably be dealt with in future PRs. I should be available again around noon pacific. Feel free to make and push small changes; I can review them when I'm online again if needed. Thanks again for all the careful checking and workshopping, @gwhitney and @katestange!

gwhitney commented 7 months ago

OK, to the best of my knowledge all of the items above this comment are resolved, except for if @katestange wants to take a look at the link to the event-handling methods in the making a visualizer doc (and perhaps express a preference for an explicit list in our docs), see https://github.com/numberscope/frontscope/pull/293#pullrequestreview-2017509401.

So I would be ready to approve for merge except for one new real concern that I think we should at least discuss before merging. The example visualizer, if you run it, just puts up a big number with a blue rectangle below it, and then as far as the person looking at it can tell, stops. There is no clue to the further behavior possible -- that person would just have to guess that maybe some keys did something, and start hitting some more or less at random. That seems at odds to me with some of the "no barriers" principles we just elucidated in our Delft meeting, and so I am not sure we want to model that kind of opaqueness of visualizer behavior in our template.

That suggests that (A) At some point going forward we may need a facility for, and a spot to display, mini-user-guide info for visualizers, if they have it; and (B) For now, for this PR, we should just display some fine print like "right and left arrow keys to navigate" below the bar, or even more briefly "(←/→ keys)" to keep the display as uncluttered as possible.

What say you, my colleagues? And @Vectornaut, congratulations on a major PR -- you should be proud of how this has come out!

katestange commented 7 months ago

OK, to the best of my knowledge all of the items above this comment are resolved, except for if @katestange wants to take a look at the link to the event-handling methods in the making a visualizer doc (and perhaps express a preference for an explicit list in our docs), see #293 (review).

It's perfect.

That suggests that (A) At some point going forward we may need a facility for, and a spot to display, mini-user-guide info for visualizers, if they have it; and (B) For now, for this PR, we should just display some fine print like "right and left arrow keys to navigate" below the bar, or even more briefly "(←/→ keys)" to keep the display as uncluttered as possible.

What say you, my colleagues? And @Vectornaut, congratulations on a major PR -- you should be proud of how this has come out!

For (A) let's add an issue and mark it UI and mention it to the Delft team; (B) yes, something visual, as unobtrusive as possible, I agree. Here's an idea for a simple visual clue, although Glen's second suggestion is perhaps simpler: https://images.app.goo.gl/BkEBS8VCPL6PsxjU6

gwhitney commented 7 months ago

Agreed on both counts. In the interest of expediency (it's well time to move this along) I just checked in a small commit that puts the text below the bar. Aaron, feel free to change it as you see best if you feel some other hint would fit in the minimalist graphic design better. At this point, when you're happy we're happy and will merge this! In fact, I will give my approval now.

Vectornaut commented 7 months ago

These changes look reasonable! I've tweaked the "never really finished" comment, fixed some links that broke when headings changed, polished the template visualizer's controls hint, and shrank the template visualizer sample image. Happy to follow up on other issues in future PRs.