search

numberscope / frontscope

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

fix: Tolerate visualizer inhabit()ing a new div before removal from old #297

Closed gwhitney closed 2 months ago

gwhitney commented 2 months ago

Refactors the VisualizerInterface to require matching inhabit() and depart() calls, both taking an HTMLElement where its view will reside. A second inhabit() before any depart() automatically calls depart() on the prior view.

Resolves #295.

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.

gwhitney commented 2 months ago

@katestange @Vectornaut I think reviewing/merging this is absolutely top priority because the current Numberscope in main is uninstallable.

katestange commented 2 months ago

I have done npm run dev and npm run build npm run preview and stress tested it by hand without causing any weird behaviour.

katestange commented 2 months ago

I see there's still discussion going on in the original issue, so I'm not sure if you want me to merge or wait?

gwhitney commented 2 months ago

I personally am fine with merging. The only point under discussion in the issue is whether it is an error or a no-op to depart() a visualizer that is not currently inhabiting anything. That is something we could always change in a later PR, and the current error behavior is "conservative" in that if it ever happens we will find out about it and can respond then.

gwhitney commented 2 months ago

So if it makes sense to you that this point doesn't have to be resolved before merging, please merge but leave the issue open. Thanks!