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

Keep new event on grid during save #145

Open
1 task done
tyler-dane opened this issue Oct 3, 2024 · 8 comments
Open
1 task done

Keep new event on grid during save #145

tyler-dane opened this issue Oct 3, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@tyler-dane
Copy link
Contributor

tyler-dane commented Oct 3, 2024

Prerequisites

  • Using an up-to-date main branch

Expected Behavior

Event rectangle is always present.

Current Behavior

Event rectangle disappears from the screen temporarily while the event is being saved

Steps to Reproduce

  1. Optional: set network to slow 3G to increase delay

  2. Click grid or all day row to create new event

  3. Notice: event disappears from screen and then reappears

Possible Solution (Not obligatory)

This is apparent only during creating a new event, because there is no existing grid event to fall back on when the draft event is disappearing. There needs to be some way to either preserve the draft event on the screen or optimistically render the regular event without waiting for a successful response from the server. If going down the optimistic rendering route, be sure to handle how to revert the event if the save failed.

Context

As a result, users are less confident that their changes will be saved.

@tyler-dane tyler-dane added bug Something isn't working needs more info Not ready for coding just yet labels Oct 3, 2024
@tyler-dane tyler-dane removed the needs more info Not ready for coding just yet label Oct 3, 2024
@tyler-dane tyler-dane moved this to In Progress in v1 Cleanup Oct 3, 2024
@tyler-dane tyler-dane moved this from In Progress to Todo in v1 Cleanup Oct 3, 2024
@tyler-dane tyler-dane added enhancement New feature or request and removed bug Something isn't working labels Dec 2, 2024
@tyler-dane tyler-dane removed this from v1 Cleanup Dec 2, 2024
@tyler-dane tyler-dane changed the title Event disappears temporarily during initial save Keep new event on grid during save Dec 6, 2024
@murilo9
Copy link
Contributor

murilo9 commented Dec 8, 2024

May I work on this one?

@tyler-dane tyler-dane moved this from Ready to In progress in 🏗 Compass Roadmap Dec 8, 2024
@tyler-dane
Copy link
Contributor Author

You bet! Assigned it to you and marked it as In Progress

@murilo9
Copy link
Contributor

murilo9 commented Dec 8, 2024

I'm having some difficulty understanding the event creation flow. In the EventForm component, there is the onSubmitForm function that calls onSubmit (from props). I was expecting that this onSubmit function would be asynchronous so I could wait for it to finish before removing the draf event (which I still don't know exactly how it gets deleted). But as I have seen, it only leads to calling draftUtils.submit, which dispatches createAsyncSlice.request, which in turn only changes some boolean state variables (isProcessing, isSuccess, error). I have no idea what happens next, nor how/when the backend call is made.

@tyler-dane
Copy link
Contributor Author

tyler-dane commented Dec 9, 2024

Pretty close! The only piece that's missing is the connection between Redux and Redux Saga.

See event.slice.ts:createEventSlice, which is triggered by the sidebar and grid.

That slice then callsevent.sagas.ts:createEvent, which triggers the API call, normalizes the response, and puts it into the correct Redux store.


Here's how to think about the request flow:

  1. UI: triggers (Redux) actions
  2. Redux Saga: middleware between UI actions and Redux. Coordinates the lifecycle of a request, including the API call, normalizing the response, and adding it to Redux
  3. Redux: stores the data and exposes actions to the UI

As you're seeing, this is all unnecessarily complex and requires a lot of code. In the future, I'd like to move away from Redux Saga and towards TanStack/Remix/Next. However, that's not a top priority now. For this issue, let's just focus on using the tools and patterns that are in currently in place.

@murilo9
Copy link
Contributor

murilo9 commented Dec 9, 2024

Great. I took some time to re-read Redux Saga's docs, and things got way clearer now. I'll keep sharing the updates.

@murilo9
Copy link
Contributor

murilo9 commented Dec 9, 2024

So my plan was to remove the discard call from useDraftUtil.ts:submit, and relocate it to somewhere else: a useEffect inside useDraftUtil, that watches for changes on state.events.createEvent.isSuccess (you can check the code changes I made so far here). But that wasn't enough: the draft still disapears once I click the Save button. So there is some logic somewhere that is still clearing the draft.

There is also a point I'm a bit stuck right now: the rendering logic of Draft.tsx. As I've seen, its rendering depends on two variables: draft (which ultimately comes from draftUtil) and isDrafting (which comes from the store). If either variable is falsy, then Draft doesn't render. But what makes me confused is: can there be a draft while isDrafting is false? Can isDrafting be true while there is no draft? How we make sure both will be in sync all the time? Couldn't we tell whether the user is drafting or not solely by checking if there is a draft? (so isDrafting would be redundant and unecessary, unless It is needed somewhere else, I don't know). These are 2 variables in very different places, that gets modified by different parts of the code and lead to many side effects, so I'm afraid modifying their logic may likely introduce new bugs.

@tyler-dane
Copy link
Contributor Author

@murilo9 You make a good point about how keeping separate state values makes it tricky. Some of the state is only local (draftUtil), some of it is only in Redux, and some uses a mix of both local state and Redux state.

To keep consistent with the idea from this comment in #150, let's try to make this work without making big changes to how useDraftUtil does things. Instead, let's make the change in the Draft component. That'll help us move forward with this feature, while also saving us some time to reassess the entire state management approach.

Here is a proof-of-concept on how to do that. It copies the draft state, except when the state is null. This lets us only remove the draft from the screen when isProcessing is set to false. There are a lot of things wrong with the POC, but it does demonstrate how to preserve the grid during save without doing a big refactor to useDraftUtil

@tyler-dane tyler-dane moved this from In progress to In review in 🏗 Compass Roadmap Dec 11, 2024
@tyler-dane tyler-dane moved this from In review to In progress in 🏗 Compass Roadmap Dec 12, 2024
@tyler-dane tyler-dane moved this from In progress to In review in 🏗 Compass Roadmap Dec 12, 2024
@tyler-dane
Copy link
Contributor Author

Unassigned and moved back to Ready based on this PR comment

@tyler-dane tyler-dane moved this from In review to Ready in 🏗 Compass Roadmap Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Ready
Development

Successfully merging a pull request may close this issue.

2 participants