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 Incorrect State Update in Sketch/index.js, Typo in Image Selector #281

Merged
merged 2 commits into from
May 25, 2020

Conversation

mattxwang
Copy link
Collaborator

This PR addresses two bugs (and leaves a third unsolved, but slightly-better solved):

  1. As @jamieliu386 reported, there is a bug involving controlled vs. uncontrolled inputs when the Create, Edit, or Delete Sketch modals are left. This is because previously, they (for some reason) updated the input values in the modal after they are closed, which is when the internals are unmounted by react-modal. I fixed this by... not doing that. This resolves the problem for EditSketch and ConfirmDelete, but there are lingering problems in CreateSketch (which I will not deal with in this PR).

  2. There was a small typo involving maxWidth instead of max-width for a CSS-in-JS property in ImageSelector; this fixes that typo.

A general observation is that all of these modals are really really poorly coded and probably need an entire refactor. Slow and steady wins the race.

fixes uncontrolled input problem in EditSketch and Delete Sketch

Note: createSketch still has a problem
@mattxwang mattxwang added the bug Something isn't working label May 23, 2020
Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

Hey Matt, thanks for handling this card. I noticed that you mentioned createSketch still has the problem so I tested locally and observed the errors once before it randomly stopped throwing the messages. Really strange stuff, can you reproduce?

@mattxwang
Copy link
Collaborator Author

I noticed that you mentioned createSketch still has the problem so I tested locally and observed the errors once before it randomly stopped throwing the messages. Really strange stuff, can you reproduce?

Do you mean for the CreateSketch workflow? My guess on the problem is that the ReactModal has an onClose that changes the state of the Sketch/index component, but when the sketch is created, the CreateSketchModal component handles the redirect itself: thus, you get a race condition of sorts where the page may or may not still be on Sketch/index when () => this.setState({ createSketchModalOpen: false }) (line 178 in Sketch/index) is called. So you'll get inconsistent results, depending on how quickly the Redirect might be triggered.

Solution probably involves the root Sketch/index to handle the redirect so it can clean itself up properly; but, I think in general the problem is that the modals (especially CreateSketch) all need some refactoring.

Copy link
Contributor

@krashanoff krashanoff left a comment

Choose a reason for hiding this comment

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

Solution probably involves the root Sketch/index to handle the redirect so it can clean itself up properly; but, I think in general the problem is that the modals (especially CreateSketch) all need some refactoring.

Yeah, that seems to be the case. Well I think this fix is acceptable for the time being, though we will eventually need to move all redirect handling up a level in component hierarchy relative to what we are currently doing. In any case, LGTM, but would still like to get input from @jamieliu386

@jamieliu386
Copy link
Contributor

LGTM as well!

@krashanoff krashanoff merged commit 06ffa37 into master May 25, 2020
@krashanoff krashanoff deleted the fix-edit-sketch-bugs branch May 25, 2020 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants