Initial version of the drag edge to canvas feature #661
Conversation
There was a problem hiding this comment.
Good job with the initial version. From my perspective there is a room for an improvement when it comes to code style/structure/... and especially design choices like how do we deal with dialogs and how do we structure our code.
I've commented on several issue, please try to address them or provide explanation. For some, like use of TODOs, I've not repeated my comment for all relevant lines.
| @@ -130,7 +130,7 @@ export const EntitiesOfModel = (props: { | |||
| }; | |||
|
|
|||
| const handleAddConcept = (model: InMemorySemanticModel) => { | |||
| openCreateClassDialog(model); | |||
| openCreateClassDialog(undefined, model); | |||
There was a problem hiding this comment.
Can you use "null" instead of an "udefined"? Even better would be to have an extra method without the first argument as not it is not clear why the first argument is "undefined".
There was a problem hiding this comment.
I will try to use null for invalid values from now on.
Split into 2 methods - openCreateClassDialogWithCallback, openCreateClassDialog
Should I in future use the Type suffix for types? like here: CreateClassCallbackType or just CreateClassCallback or should I use it depending on the actual name?
| @@ -20,8 +20,10 @@ export const useCreateClassDialog = () => { | |||
| const { isOpen, open, close, BaseDialog } = useBaseDialog(); | |||
| const [model, setModel] = useState<InMemorySemanticModel | null>(null); | |||
| const [position, setPosition] = useState<{ x: number; y: number } | undefined>(undefined); | |||
| const [onCreateClassCallback, setOnCreateClassCallback] = useState<((newEntityID: string) => void) | undefined>(undefined); | |||
There was a problem hiding this comment.
Why do we need this here?
There was a problem hiding this comment.
The reason is that when drag edge on canvas and select the option "Create new Class", we want to immediately after that open the dialog to create connection between the 2 nodes.
I felt like this is better option. Alternative implementation could be to have useEffect in the visualization.tsx with dependency on isOpen state. But the we would have to find out if new class was created and if so, which one.
| import { tailwindColorToHex } from "~/app/utils/color-utils"; | ||
|
|
||
| const getDefaultColor = () => { | ||
| return "#069420"; |
There was a problem hiding this comment.
This should be part of the configuration.
There was a problem hiding this comment.
I agree, that being said I stole it from here and I haven't quite checked code to see what is the reason for having 4 default colors, so for now I just use local "copy" of the default color.
| }; | ||
|
|
||
| export const useSelectClassesDialog = () => { | ||
| const { isOpen, open, close, BaseDialog } = useBaseDialog(); |
There was a problem hiding this comment.
I've started changing how dialogs works. It's not online yet, but for future it would be best to have only a single way of working with dialogs.
There was a problem hiding this comment.
We can discuss that later, for now I will try to avoid dialogs. That being said I tried to rewrite the SelectClassDialog in a similiar way to the CreateClassDialog (as in the latest version in main).
| open(); | ||
| }; | ||
|
|
||
| // TODO: Maybe could be optimized later, when I get to know React better |
There was a problem hiding this comment.
Please do not put TODOs into the code, create an issue instead. As using both TODOs and Issues give us two places where we keep track of things to do. When not sure, you can always ask other to help you with a decision.
There was a problem hiding this comment.
Sorry about that his bad habit of mine. I will probably keep these TODOs somewhere on the side or ask others in some instances.
Maybe I will create 1 general issue with all the stuff I am unsure about, but I don't like putting such stuff into issues, I usually put the TODOs in code because they are "hidden" from the public.
| const onCreateClassCallback = () => (newEntityID: string) => { | ||
| // With timeout the dialog is in the same place, but it takes a moment to get there | ||
| setTimeout(() => setEntityIDsToConnectTo([...entityIDsToConnectTo, newEntityID]), 200); | ||
| // setEntityIDsToConnectTo([...entityIDsToConnectTo, newEntityID]); |
There was a problem hiding this comment.
Please no commented code in the repository.
There was a problem hiding this comment.
Ok, I will try to always remove the other variants of code, before final commit.
| setEntityIDsToConnectTo([...entityIDsToConnectTo, ...newEntityIDs]); | ||
| }; | ||
| const onCreateClassCallback = () => (newEntityID: string) => { | ||
| // With timeout the dialog is in the same place, but it takes a moment to get there |
There was a problem hiding this comment.
Do you know why? Does it have to be 200 ms .. ?
There was a problem hiding this comment.
I changed it to 100ms, 200 was too long.
It doesn't have to be 200ms, can be even 0. I don't know what is the issue. To explain it. When there is no timeout, the dialog doesn't behave as dialog and instead of being put in the "middle" it is put on the place, where it would normally be if it wasn't dialog but normal component inside DOM tree, if that makes sense.
... Maybe it will be fixed with the new version of dialogs you are working on. I think that it might be related to the BaseDialog or maybe to the way I am using it. Since I had the same issue in the Select classes dialog, but after reimplementation with ModalDialog, it works as it should.
| const connection = { | ||
| source: nodeStartingConnection?.handleType === "source" ? nodeStartingConnection?.nodeId : (entityIDsToConnectTo.shift() as string), | ||
| target: nodeStartingConnection?.handleType === "target" ? nodeStartingConnection?.nodeId : (entityIDsToConnectTo.shift() as string), | ||
| sourceHandle: null, // TODO: Maybe will have to specify handlers later in development |
There was a problem hiding this comment.
Again no TODOs use issues istead if its something to do.
There was a problem hiding this comment.
Removed.
This TODO is related to the edge routing, if we in future use some edge routing where ports actually matter (i.e. not the current implementation, where the closest one is used), then this would need to have set handlers.
| ℹ | ||
| </div> | ||
| <div className="mx-1"> | ||
| <button title="download svg of the canvas" onClick={exportCanvasToSvg}> | ||
| <button title="Download svg of the canvas" onClick={exportCanvasToSvg}> |
There was a problem hiding this comment.
Try to move towards using the localization file. Our goal should be to get all strings outside of the components code.
There was a problem hiding this comment.
I will try to do it for all the new ones I create. Not sure if I will also do it for the old ones. Should I at least mark the old ones with our favorite TODOs (like TODO: localization) whenever I notice one, so at least somebody can quickly skim through them when he has time?
| 🖼 | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </Panel> | ||
| <Background gap={12} size={1} /> | ||
| </ReactFlow> | ||
| {isCanvasMenuOptionsOpen && <CanvasMenuOptions openSelectClassDialog={() => openSelectClassDialog(onSelectClassesCallback)} openCreateClassDialogHandler={openCreateClassDialogOnCanvasHandler} />} |
There was a problem hiding this comment.
Do not do this .. instead be more explicit:
{isCanvasMenuOptionsOpen ? .... : null}
It is better with the isCanvasMenuOptionsClose variable.
Or even better, render it every time and just be sure that CanvasMenuOptions returns null when not visible.
There was a problem hiding this comment.
The null is nice and clever solution, I used it for the MenuOptions (and changed it on all relevant places), but I am not sure if I should do it now, because it makes sense to do it for all the pop-up-like elements at once, since there are many places where the && construct is used.
The null solution at least for me has one drawback. That since we are no longer explicitly checking in the DOM with isOpen, somebody who isn't familiar with the code can be confused ,why there is defined component which isn't present on the screen at all times (meaning there is harder mapping between the DOM in code and what is actually on screen all the time). Don't know if that makes sense.
Deploying dataspecer with
|
| Latest commit: |
7048107
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://79f6b5f8.dataspecer.pages.dev |
| Branch Preview URL: | https://feature-cme-drag-edge-on-can.dataspecer.pages.dev |
|
I tried to address all the comments in both code and the comments. We can go through some of them on meeting, so I guess that there is no need to answer all of them. |
|
I moved the feature to controller file, some more refactoring and added some TODOs (maybe put them to my private repository as issues? but I feel like these makes sense to be kept in code for now), which are related to the future dialog representation. |
|
Closed for now, It'll be back. |
It seems to work, but maybe the selection of existing classes will need some UI refinements.