thuiop / magium-dev

MIT License
13 stars 4 forks source link

Add Export and Import feature #61

Closed himanshunaidu closed 1 month ago

himanshunaidu commented 2 months ago

Local_Save_Load

himanshunaidu commented 2 months ago

This should be merged only after the pagination feature is merged, so that I can simply copy those changes into this PR

himanshunaidu commented 2 months ago

@thuiop I am forcing a reload everytime a json save file is restored. Is that fine? I see that in outline.ejs, you are already giving the extension.

I also have a question based on this, for the saves.ejs file: If the above reload setup is fine, would it be better to instead just trigger a reload for the 'Save' button in the saves.ejs page as well, instead of adding an extension?

thuiop commented 2 months ago

Wait, I am not sure about the workflow here. Why is there a separate page for importing/exporting rather than have it in the regular save page? Also, I would probably have gone with copying a serialized save in the clipboard, but saving to a file is also fine. Otherwise, reloading works, but this is just hiding the behavior; reloading will trigger the call to the extension in the outline.ejs. I do not really see a reason to reload the whole page while the "content" object could be swapped normally as is done in the saves.ejs file.

himanshunaidu commented 2 months ago

Right. So the idea was to somewhat mimic the cloud saves menu page, which is why I had a separate page.

himanshunaidu commented 2 months ago

So the File reading is asynchronous. Which is the desired behavior of course. This means that we cannot really hope to use a set up like this on the input of type file:

<input type="file" hidden
                id="file_<%= i %>"
                hx-on::before-request="restoreLocalStorageSave('save<%= i %>', 'file_<%= i %>')"
                hx-post
                hx-trigger='change'
                hx-ext="submitlocalstorage"
                hx-target="#content"
                hx-swap="innerHTML"
/>

What we can do however is, to manually make an ajax call, instead of reloading, in the following code.

reader.onload = function(e) {
            let data = JSON.parse(e.target.result);
            localStorage.setItem(save_key, JSON.stringify(data));

            window.location.reload();
}

Would that be the way to go?

thuiop commented 1 month ago

Ah, I see the issue. A better way would be to change the hx-trigger to trigger on a custom event instead, and fire that event instead of using reload.

himanshunaidu commented 1 month ago

Got it! I was hoping that the reload feature is fine, but it makes perfect sense. We are effectively hiding the behavior. I was also hoping there was a specific HTMX setup i was missing, I guess that may not be the case.

Sure thing then. I'll create a custom event to handle this. Let's see how it works after that.

thuiop commented 1 month ago

You can use the htmx JS API to trigger it I belive, with something like htmx.trigger(...) (I am not sure of the exact syntax again).

himanshunaidu commented 1 month ago

Greetings @thuiop @ZiClaud @lpcdragon @Colaboi2009 , Apologies for the delay on this update. Had several personal issues to deal with. I have updated the page to instead trigger a custom event.

I also copied the first version of pagingation from the saves page. I assume we will improve the logic later, but for now, imo this is a decent first iteration, as mentioned in the saves page pagination PR.

Can this be reviewed?

thuiop commented 1 month ago

Looks good to me for the actual functionality. I think it would be better to frame it as import/export in the text: the regular saves are also local, so this may confuse users. I also still think this could go in the regular save page but this is fine for now.

himanshunaidu commented 1 month ago

Alright, I'll call it Local Import/Export Menu for now. If anyone has an issue, they can change it later.

himanshunaidu commented 1 month ago

But I'll rather just change the label instead of changing the paths, and the backend naming convention. I feel like that shouldn't cause a problem. But if it is still bothersome, I can later raise a separate PR that removes the term local_saves entirely.

himanshunaidu commented 1 month ago

Merging this for now. Please let me know if anything else is required.