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

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

Closed
wants to merge 2 commits into from

Conversation

alvinosh
Copy link
Collaborator

@alvinosh alvinosh commented Apr 17, 2024

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.

@alvinosh alvinosh marked this pull request as ready for review April 17, 2024 11:36
Venryx added a commit that referenced this pull request Apr 17, 2024
@Venryx
Copy link
Collaborator

Venryx commented Apr 17, 2024

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 Venryx closed this Apr 17, 2024
@Venryx
Copy link
Collaborator

Venryx commented Apr 17, 2024

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.

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

Successfully merging this pull request may close these issues.

2 participants