Open Ri-Sharma opened 9 months ago
I think it’s intentional that some files like index.html should not be allowed to be deleted. Because there’s no code to run if there’s no files. Ideally we should have some sort of toast that tells the user that index.html cannot be deleted if they try to delete it.
Preventing deletion of the “last file” specifically doesn’t make a lot of sense because no files will be loaded/executed anyways if there is no index.html. The sketch preview is a webpage so it must have an index.html, which can optionally load additional .js or .css files.
It’s never intentional that the app crashes 😂. You should open the console and see what the error is there.
Editor
component will crash if this.props.file
is undefined
.checkPropTypes.js:20 Warning: Failed prop type: The prop
file
is marked as required inEditor
, but its value isundefined
. in Editor (created by Connect(Editor))Uncaught TypeError: Cannot read properties of undefined (reading 'fileType') at Editor.render (index.jsx:507:1)
The above error occurred in the
<Editor>
component:
We can either:
Editor
component to handle the case where this.props.file
is undefined
.Editor
if there is no file. In reality this won't work because the sidebar expand button is part of this Editor
component.this.props.file
is always defined by making sure that the user cannot delete all files.We are attempting to do option 3 right now. You have found a loophole where the user is able to delete the final file by deleting the folder which contains it.
There are some additional non-fatal errors which are happening right now.
index.html
The "preview app" which is executed in the <iframe>
crashes.
TypeError: Cannot read properties of undefined (reading 'content') at _t (EmbedFrame.jsx:214:53) at EmbedFrame.jsx:293:23
I thought that it was intentional that you can't delete the last file but that might just be a bug after all! It turns out that the Redux reducer cannot handle the resetSelectedFile
action if the passed id
is the last id, as there is no possible file to select. This crashes the onClick
event handler and prevents the deleteFile
action from ever executing.
ide.js:59 Uncaught TypeError: Cannot read properties of undefined (reading 'id') at ide.js:59:63 at index.js:8:18 at Object.resetSelectedFile (bindActionCreators.js:3:12) at t.handleClickDelete (FileNode.jsx:158:18)
@lindapaiste Yeah when deleting the last file the resetSelectedFile
function throws error.
export function resetSelectedFile(previousId) {
return (dispatch, getState) => {
const state = getState();
const newId = state.files.find(
(file) => file.name !== 'root' && file.id !== previousId
).id;
dispatch({
type: ActionTypes.SET_SELECTED_FILE,
selectedFile: newId
});
};
}
Because of the expression in const newId
we are unable to delete the last file, because whenever we try to delete the last file the state.files.find()
try to find any file that is not current file and is also not root and in case of last file there is no such file so we are receiving an error here. Hence preventing the file from deleting.
But in case of deleting the 'Last Folder' the when the resetSelectedFile
runs it selects the file that is inside it (in case the folder is empty the behavior will be the same as when we try to delete the 'last file') so it wont throw an here. But when the state is trying to access the file that is selected (which is already deleted when we deleted the folder) it just crashes the app.
Some Points :- 1) There is also an minor bug when we select an folder the editor goes blank. But it should just show the previous selected file. (Like in VS Code)
2) I think the above bug is related to this issue because the editor should go blank only when there is no files present. (Or we can add an placeholder)
3) I wish you a very Happy New Year.
In order to keep the previous file open we probably need to separate what is currently one property in the state into two properties. One property that tells us which file or folder to highlight in the sidebar and other that tells us which file to open in the editor. Our SET_SELECTED_FILE
action would check if the id
to select is a file or a folder. If it's a file then we update both the highlighting and the editing. If it's a folder then we update the highlighting but don't change the editing. FYI at some point I want to refactor the whole files
reducer using RTK and I'm imagining that selectedId
would be a single property rather than having an isSelectedFile
property on each file.
I'm relatively new to open source, and I believe that adding a snackbar would enhance user experience by providing clearer feedback on issues. I would like to contribute by addressing this enhancement. Could you please assign me the relevant issue? I am eager to start working on it.
In order to keep the previous file open we probably need to separate what is currently one property in the state into two properties.
@lindapaiste, My suggestion is we don't need to highlight the folder when it is being clicked, we can just Toggle the show/hide children whenever an folder is being is selected.
Currently the Show/Hide folder children is only working when we click the following button, but we can make it such that it will work if we press wherever (in the folder area of course). Open Folder Close Folder
@lindapaiste I think the root index.html should be prevented from being deleted here and some sort of alert or toast should be displayed to the user, can you assign this issue to me?
@Pankaj3112, Yeah an alert like an toast would be great but I think we have to solve the underlying issue here.
Here is an old issue: #1070. It seems like the consensus both there and here is that the user should not be able to delete the index.html
file. We can either:
A) Remove the "Delete" option from the dropdown menu
B) Keep the "Delete" option visible, but when it's clicked we show a toast explaining that index.html cannot be deleted.
@lindapaiste, after some exploration, I've gathered the following insights:
We can tackle the above problem by updating the renderSketch
function from client\modules\Preview\EmbedFrame.jsx
in a following manner,
function renderSketch() {
const doc = iframe.current;
if (htmlFile === undefined) { +
dispatch(showToast('HTML File not found')); +
dispatch(stopSketch()); +
doc.src = ''; +
} +
else if (isPlaying) {
const htmlDoc = injectLocalFiles(files, htmlFile, {
basePath,
gridOutput,
textOutput
});
const generatedHtmlFile = {
name: 'index.html',
content: htmlDoc
};
const htmlUrl = createBlobUrl(generatedHtmlFile);
// BRO FOR SOME REASON YOU HAVE TO DO THIS TO GET IT TO WORK ON SAFARI
setTimeout(() => {
doc.src = htmlUrl;
}, 0);
}
else {
doc.src = '';
}
}
"+" indicating addition of line
What is your operating system?
Windows
Actual Behavior
The issue can be divided into two parts :-
1) Not being able to delete the last file.
https://github.com/processing/p5.js-web-editor/assets/80570105/f38c0437-b8c3-47be-918b-90ba00aa6fa6
2) Deleting the last folder causes the app to crash.
https://github.com/processing/p5.js-web-editor/assets/80570105/7a6f8f15-d7dc-402c-8f14-6330234321e0
Hey @lindapaiste , @raclim can you assign this issue to me as I am already working on this.
Expected Behavior
1) We should be able to delete the last file just like other files. 2) Also we should be able to delete the last folder without any problems.
Steps to reproduce
Steps:
For the first one try to delete all the files that are available.
For the second one create an folder and just some files into it, then delete all root folder files. Now if you delete the created folder the web app would crash.