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

Add isSaving state to project title and description #519

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

jeffdaley
Copy link
Contributor

Adds an isSaving spinner to the project title and description fields.

Copy link
Contributor

@jfreda jfreda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this correctly, I'm not sure it would work if someone were to very quickly save a title and then a description because this is tracking each saving state separately and we're not doing any enqueueing of the async tasks (and the save function as-is wouldn't be able to differentiate between saving each field). It seems like the easiest thing would be to just make one saving state (like we do for the sidebar), or else make changes to support saving each field separately. Let me know if you think I'm misunderstanding the logic though.

@jeffdaley
Copy link
Contributor Author

If I'm understanding this correctly, I'm not sure it would work if someone were to very quickly save a title and then a description because this is tracking each saving state separately and we're not doing any enqueueing of the async tasks (and the save function as-is wouldn't be able to differentiate between saving each field). It seems like the easiest thing would be to just make one saving state (like we do for the sidebar), or else make changes to support saving each field separately. Let me know if you think I'm misunderstanding the logic though.

I changed the save task to be restartable to handle the edge case where a save attempt is made while another is in progress. In this case, you'd see two spinners. For general usage (editing one at a time, respecting the short isSaving state), a single spinner looks better.

@jfreda
Copy link
Contributor

jfreda commented Dec 20, 2023

I changed the save task to be restartable to handle the edge case where a save attempt is made while another is in progress. In this case, you'd see two spinners. For general usage (editing one at a time, respecting the short isSaving state), a single spinner looks better.

Okay, yeah agree about the UX, but I was just thinking about the functionality. I'm wondering if there still might be an edge case where the first task fails and the request gets lost, but I'm okay with leaving that for later if it's a possibility.

@jeffdaley
Copy link
Contributor Author

Ah, looking back at it, I was under the assumption that we always saved title, description and status regardless of which attribute was changing. That isn't the case so I'm switching to an enqueue task. Sorry for confusing things.

@jeffdaley jeffdaley merged commit bac43a6 into main Dec 20, 2023
2 checks passed
@jeffdaley jeffdaley deleted the jeffdaley/projects-saving branch December 20, 2023 21:51
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.

None yet

2 participants