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

Draft for Jarring Animation #6 fix #10

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ghassanachi
Copy link
Contributor

Proposed fix for #6.

I'm leaving this as a draft for now, since there are a couple choices I made that might need to be validated before a review is in order. And parts of the code need to be fixed (some of it is just indentation issues, discussed below).

To make testing the UI/UX changes a little easier, I've set up the current version and new version on a dev box (hope you don't mind me hosting them, I'll tear them down once the PR is merged). The poll rate has been dropped 1000ms on the host view to make animation overlap more frequent.

debounced: https://new.ghassanachi.com/
current: https://old.ghassanachi.com/

The backend is running in debug/local mode, so you can use the secret endpoint.

debounced_secret: https://new.ghassanachi.com/event/00000000-0000-0000-0000-000000000000/secret
current_secret: https://old.ghassanachi.com/event/00000000-0000-0000-0000-000000000000/secret

Questions

  1. I was wondering what your thoughts on adding a formatter (eg: prettier) for the client code. With different editor setups, i've had to turn off lsp formatting and manually indent things, which doesn't always work with whitespace because of different tab-size settings). Adding a formatter for the client should resolve the issue and guarantee consistency across machines.
  2. I decided to try out the solution with the derived store, which seems to work well, but I wanted to check that we are okay with this change?
  3. I've also experimented with adding some basic transitions in addition to the animate directive. Let me know what you think or if we should remove them? (Currently they are just there for UI/UX, but we could use the "on:outroend" event to drive the redraw code, instead of the fixes animationTime value I set up.

@jonhoo
Copy link
Owner

jonhoo commented Jan 21, 2023

On the whole I think I like this approach! It's currently a little hard to review though since there are so many things going on at once. Here's what I think we should do, in order, and in separate PRs:

  1. Run a linter on client and a check for that in CI
  2. Move event to a store
  3. Factor out functions into the helpers module
  4. Make the changes needed for debouncing
  5. Make the stylistic changes you're proposing

Also, thanks for setting up the UI preview server — that was very helpful!

@ghassanachi
Copy link
Contributor Author

Sounds like a good plan (I would have worked a little more on the code the last few weeks, but I was travelling for the holidays)

I'm a little busy at the moment, but I'll try to get to it as soon as I have a little time (though if anyone sees this, is free and wants to take point, please go ahead).

And you're welcome for the preview servers :). The config is very simple just using Dockerfiles, docker-compose and Caddy, but if anyone wants to use them to host UI preview(s), I'll happily share them.

@jonhoo
Copy link
Owner

jonhoo commented Jan 28, 2023

No rush at all! I've been slow enough to get back to you that the least I can do is let you take your time 😅

@jonhoo
Copy link
Owner

jonhoo commented Dec 17, 2023

@ghassanachi Looks like with all the other bits merged, this now has a few conflicts.

Also, given you know more about the JS side of things than I do, any chance you could take care of the upgrade in #59 and #61?

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