fix(UX): opening panel while assembly setup is open forces refresh [SYNTH-120]#1315
fix(UX): opening panel while assembly setup is open forces refresh [SYNTH-120]#1315Dhruv-0-Arora wants to merge 15 commits into
[SYNTH-120]#1315Conversation
If a user opens a panel while assembly setup is open, the assembly setup panel will close and you will be unable to open another panel without refreshing Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Dhruv-0-Arora
added
the
ui/ux
Dhruv-0-Arora
force-pushed
the
darora1/120/spawn-asset-bug
branch
from
23cd10b to
5139fb4
Compare
|
Note This moves the logic of blocking the 'import' panel from being opened when 'Assembly Setup' panel is open to |
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Dhruv-0-Arora
force-pushed
the
darora1/120/spawn-asset-bug
branch
from
5139fb4 to
f28ef09
Compare
…t-bug Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
PepperLola
left a comment
PepperLola
left a comment
There was a problem hiding this comment.
This now blocks any panel from opening while an import is in progress which, although potentially inconvenient, fixes the issue where you can start an import, click configure assets, and then the panel is dismissed but other panels are blocked from opening. I also don't really like including logic specific to a single panel in the UIProvider for the whole UI - maybe it could make sense to put a blocking flag on a panel that prevents other panels from opening while it's open?
rutmanz
left a comment
rutmanz
left a comment
There was a problem hiding this comment.
I'm inclined to agree with @PepperLola, it feels really weird to put logic specific to one panel into the UIProvider file. If you can't put it in the panel's file, then I like his idea of making a generic "blocking" property that prevents other panels from opening at the same time
AlexD717
left a comment
AlexD717
left a comment
There was a problem hiding this comment.
The edit control buttons are now fully blocked from opening (similar story with new input). These should either be fully removed (not advised) or some other solution implemented.
|
@AlexD717 I personally think we should remove it. I can't think of a use case of using the edit feature and also when you click it in dev, it finishes spawning the assembly which I think is extremely confusing especially if you are trying to actively configure the inputs for the assembly (and then it just completes setup while you are still trying to set it up). |
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Dhruv-0-Arora
force-pushed
the
darora1/120/spawn-asset-bug
branch
from
59582d0 to
bfba443
Compare
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Dhruv-0-Arora
force-pushed
the
darora1/120/spawn-asset-bug
branch
from
bfba443 to
80826f7
Compare
| if (blockingPanel) { | ||
| const msg = blockingPanel.props.blockingMessage ?? "Close the current panel before opening another." | ||
| addToast("warning", msg) | ||
| return blockingPanel.id |
There was a problem hiding this comment.
I'm not sure this behavior makes sense; openPanel is supposed to return the id of the panel that was opened - if no panel was opened (since it was blocked), then shouldn't the function return null? I know this was the behavior before with existing but on retrospect I'm not sure it makes much sense. Would have to look at where the result is used and if we depend on this behavior or we expect it to be non-null.
There was a problem hiding this comment.
Changing the type signature of openPanel to return void doesn't seem to cause any type errors, and I can't find an instance of the panel id being used
There was a problem hiding this comment.
I think it makes sense to return blockingPanel.id just because if this is ever used at any point, it would make sense to return the currently open panel (the one that is blocking other panels from opening).
There was a problem hiding this comment.
I mean I don't really agree with that bc places opening the panel don't have any reason to suspect that the panel is blocked. If I try to open a configuration panel and use the ID that comes back to close it later then I'm actually closing the import panel which isn't really logical. I would change the method signature to just make the returned id optional and return null here. It's not currently used anywhere so there shouldn't be anything else you have to handle
| if (blockingPanel) { | ||
| const msg = blockingPanel.props.blockingMessage ?? "Close the current panel before opening another." | ||
| addToast("warning", msg) | ||
| return blockingPanel.id |
There was a problem hiding this comment.
Changing the type signature of openPanel to return void doesn't seem to cause any type errors, and I can't find an instance of the panel id being used
…Panel.tsx Co-authored-by: Zach Rutman <zarutman+git@gmail.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
rutmanz
left a comment
rutmanz
left a comment
There was a problem hiding this comment.
Other than the ID thing, Lgtm
Dhruv-0-Arora
changed the title
SYNTH-120[SYNTH-120]
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
AlexD717
left a comment
AlexD717
left a comment
There was a problem hiding this comment.
Can the new input button also be removed? I don't think it makes sense to let them make a new input, but then not customize it.
Other than that, looks good to me.
Signed-off-by: Dhruv Arora <dhruv.arora1@autodesk.com>
4 participants
Symptom
SYNTH-120
If a user tries to spawn an assembly before completing with another one, they are hit with this error. Even with the panel closed and the assembly completed, there is no way to spawn another assembly.
Solution
There was a missing react hook dependency, so I added it.
Before merging, ensure the following criteria are met: