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

Fix: Ensure EditUserModal can't be closed until the crop is finished or canceled #1116

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

Conversation

Nostromos
Copy link

Fixes #896 and very similar to #911.

@ddfridley
Copy link
Collaborator

ddfridley commented Oct 14, 2024

@Nostromos to fix the issue you are working on, is it necessary to make these changes? Changing the version of python impacts the whole system, and would require more extensive QA testing before release. As for the @flow changes, same question, but I don't know @flow and if effects run time, or just build time. I know these seem like simple changes, but this is a product in production and we need to be very careful about changes. QA testing and releasing to production is a big task, we want to make the changes as focused as possible. Thanks!

@Nostromos
Copy link
Author

Nostromos commented Oct 14, 2024

Hey @ddfridley - Thanks for the engagement! FYI this is still a draft so not ready to review yet. Childcare fell through today so halfway through my commits had to stop and go home. As far as your comment...

Changing the version of python impacts the whole system, and would require more extensive QA testing before release

This only changes the python version for the pre-commit package config, not python for the whole system. I could not get working otherwise and pre-commit is required to commit anything. I plan to remove this commit before marking this PR ready for review. I'll take some time after to figure out why pre-commit isn't recognizing my python 3.8 install.

As for the @flow changes, same question, but I don't know @flow and if effects run time, or just build time. I know these
seem like simple changes, but this is a product in production and we need to be very careful about changes. QA testing and releasing to production is a big task, we want to make the changes as focused as possible.

We're aligned. These flow changes shouldn't affect run or build time as far as I can tell. Reviewing my own builds doesn't show any change.


I'm being really careful not to change more than necessary and to keep the fix in scope. I'm also referring to the other PR submitted for this issue and getting ahead of the issues raised there. Honestly, that fix looked good but I need the practice and its good exposure to the codebase for me.

tl;dr Check back when this PR is out of draft.

@Nostromos
Copy link
Author

Nostromos commented Oct 21, 2024

Update: My wife and I managed to find childcare so I should be back at this tomorrow. Major apologies for the delay but kiddo is really little so its non-stop.

@ddfridley
Copy link
Collaborator

@Nostromos Sorry for the slow responses. I'm back from vacation and so is Marlon. How is it going?

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.

Perceived failure to save profile picture
2 participants