search

debate-map / app

Monorepo for the client, server, etc. of the Debate Map website.
https://debatemap.app
MIT License
73 stars 16 forks source link

#37: "Saving..." -> "Saved!" text next to the Save button #293

Closed alvinosh closed 6 months ago

alvinosh commented 6 months ago

There is a slight issue with the implementation. After a node revision is added, every component seems to be unmounted and then remounted, causing a short "flicker" and the state to reset entirely.

The only way I can see of fixing this issue would be to have the feedback live outside of the component (such as a toast system) or to somehow not remount the component on every render.

Venryx commented 6 months ago

Looks good, thanks!

I manually merged it, adding a small tweak to the styling (brighter green for the checkmark), and leaving out the change to yarn.lock.

Regarding yarn.lock: On my own machine I use yalc + zalc, which is basically an alternative to symlinks/npm-link/yarn-link. While it's the best solution I've found so far for making local changes to dependencies and subdependencies (to test changes without having to publish each time), it has the negative side-effect of causing the yarn.lock to differ between developers, even if no actual change to the targeted dependency versions took place.

So for future pull-requests, probably leave out the yarn.lock file, unless of course you're adding/updating a dependency. (if you do include the file though that's also not really a problem; it's just noise for now unfortunately, until I set up some automation to have my yalc/zalc changes temporarily undone when I make commits myself)

Venryx commented 6 months ago

Oh and regarding the remount issue: This is only "sort of" a bug. Technically it's not ideal behavior, but it's an expected side-effect of the way that debate-map currently handles "in-progress loading" of new information (in this case, the new node revision).

It has a custom layer (implemented in the custom mobx-graphlink library) that "requests" the new revision, and then while it's waiting for that new data, it replaces that section of the tree with a loading icon; because of this replacement, that part of the subtree gets unmounted, then a new subtree is created once the data loads in.

That loading logic works kind of similarly to React Suspense (https://react.dev/reference/react/Suspense), in that it uses a "loading ui", and signifies loading by means of "throw" which propagates up the call/render tree until a "loading handler" catches it -- except that the custom loading logic doesn't yet support the useDeferredValue hook, which is the React Suspense way of keeping a subtree showing the old data in the "loading state" in order to keep an unmount->remount from happening.