Folders to organize scenes #7777 #8165
Conversation
|
|
||
| this.props.expandFolders([this.getId()]); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
There were two comments associated to this setTimeout and the value. Can you please add them back? Moving code should not have their comments lost.
There was a problem hiding this comment.
@anasiez You marked this GitHub comment as resolved but I still don't see any comment added back. This is taking a bit of time to review the PR so I need GitHub comments to stay open if they are not resolved.
4ian
left a comment
4ian
left a comment
There was a problem hiding this comment.
Hi!
First, thanks for opening this, appreciate your work on this.
To have this considered for a potential inclusion in GDevelop, we need some attention to quality and details in the code and in the app, here is a quick list from what I've seen just looking quickly:
- You've added some comments in German. All the code of GDevelop is commented in English. For each comment you added, consider if:
- 1 (better): remove it if it does not add any information to the exist code.
- 2 (otherwise): translate it in english
- On the contrary, you've sometimes removed existing comments. Can you put them back? Any comment in the existing code must be preserved, especially those explaining hacks or things to know. For example, a setTimeout is something that we should in theory never use. If there is one, you must absolutely keep the comments.
- I've tried it and it works for scenes (great! :)) but external layouts and external events don't have 1) the menu item to add folders and 2) they have an extra triple dot menu that is useful.
- Can you fix this and confirm you've tried/tested this manually?
See, nothing here:
While it works here:
- Can you fix this and confirm you've tried/tested this manually?
Take a careful look at all my comments, ensure the external layout/events and scenes folders can be used from the UI and ensure the code is as clean as possible (without losing existing information) and this would be then a great addition :) Looking forward to your changes! Thanks again! :)
|
Hi @4ian , thank you for your feedback! We have fixed the comments you have written. Looking forward to your feedback! |
4ian
left a comment
4ian
left a comment
There was a problem hiding this comment.
Thanks for the update! I made another pass. To continue I would need you to make a global check on your PR, as this is a long PR:
- Ensure all existing code that was removed still have comments and same features. Notably, I'm very concerned about MemoizedProjectManager -> this is something that was removed without anything in the PR description and I could have missed this.
- No german comments :)
- If functions are duplicated, factor them.
- Make sure there is no dead code (unused functions)
I know this is long but this is only by having a PR as minimal as possible that we will be able to get this in the codebase and then maintained over the course of multiple years :) For this, I need to ensure every single comments I made is also addressed. Otherwise I have to re-read everything again and will have to space out my reviews as it's taking a lot of time. This is a long PR so I appreciate your work! Good luck!
| * @template TFolderOrItem - Der C++ FolderOrItem Typ (z.B. gdObjectFolderOrObject) | ||
| * @param selectedFolderOrItem - Das aktuell ausgewählte Folder oder Item | ||
| * @returns {{ folder: TFolderOrItem, position: number }} - Parent und Position zum Einfügen |
There was a problem hiding this comment.
We usually don't use JSDoc for template/param/returns because Flow types are already doing the job and often the parameter name is self explanatory. You can remove this.
| @@ -1532,19 +1868,7 @@ const ProjectManager = React.forwardRef<Props, ProjectManagerInterface>( | |||
| } | |||
| ); | |||
|
|
|||
| const arePropsEqual = (prevProps: Props, nextProps: Props): boolean => | |||
There was a problem hiding this comment.
You've removed this MemoizedProjectManager implementation but:
- you did not tell about this in the PR description
- you kept the name "MemoizedProjectManager", which gives the false impression that it's memoized while it's not.
Can you either revert this change or explain why it was needed? This is where I need you to be extra careful because the comment was explaining why it was needed.
Please do the same for the whole PR: inspect every line and ensure you've not removed already existing things - unless you explain why :) Thank you!
| treeViewRef.current.openItems([scenesRootFolderId]); | ||
| } | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
I think I made the comment already but there is still the same issue :/ Comments have disappeared between your version and the old one:
These comments are important and I need to be 100% sure you've not removed anything important.
|
|
||
| this.props.expandFolders([this.getId()]); | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
@anasiez You marked this GitHub comment as resolved but I still don't see any comment added back. This is taking a bit of time to review the PR so I need GitHub comments to stay open if they are not resolved.
| } | ||
|
|
||
|
|
||
| export function collectAllItems<TFolder, TFolderOrItem, TItem>( |
There was a problem hiding this comment.
Could you explain why is this function useful?
| return false; | ||
| } | ||
|
|
||
| _collectFoldersAndPaths( |
There was a problem hiding this comment.
I see this function both here and in newIDE/app/src/ProjectManager/SceneTreeViewItemContent.js and innewIDE/app/src/ProjectManager/SceneTreeViewHelpers.js
Can you factor them?
Sure no problem. Your PR title indicates "external events and external layouts" => clean the title of the PR so that it's faithful to what's inside. Thanks! |
anasiez
changed the title
2 participants
Our student group has made the folders to organize scenes, so you can make folders, put the scenes in it and help to organize the game production better.