Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added session storage saving #25

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

himalinig
Copy link

@himalinig himalinig commented Feb 8, 2023

Summary of changes

  • added session storage saving for code on editor code changes

Testing

https://drive.google.com/file/d/1LJi7tSLezhvhadT4drfxtZX6kq7HZlgC/view?usp=sharing
^ video of manual testing

issues assoicated

  • File saving #14
  • This could be revisited so that code is stored using the history API as Jim suggested. That would be a more robust solution because the user could go backwards/forwards in history on the tab and still have their code.

@ixchow
Copy link
Member

ixchow commented Feb 10, 2023

I want to test how this interacts with the loading-via-URL feature before we merge it into the main branch.

e.g. what happens with something like:
https://textiles-lab.github.io/knitout-live-visualizer/?load=https://knit.work/code/sheet-cast-on/swgn2/js
?

(for that matter, what should happen?)

@himalinig
Copy link
Author

Here's what I think should happen:

  1. if the page is refreshed and the URL has changed to load a file or load a different file, pull code from file and overwrite session storage. Further edits are stored in session storage.
  2. If the page is refreshed and the URL has not changed, pull from session storage.

^ if this model of desired behavior is good then I can try and implement it

The other option is to go with the default behavior with the current PR, because session storage simply doesn't work with the URL loading. Here's a video of me testing it:

https://drive.google.com/file/d/1QPUcKabTAweCadsJtfMtLsuiI87M4GKm/view?usp=sharing

As you can see, it loads the code from the URL on refresh and does not track user changes. This is the current behavior without my changes anyway

@himalinig
Copy link
Author

himalinig commented Feb 10, 2023

Summary of changes:

  • I implemented what we discussed on our way over to the classroom:
    • when the URL is changed there is a new parameter added to the query string showing edited = true
    • here's me testing that functionality: https://drive.google.com/file/d/1jYimjGQIWp20yvlWYIWR04arVHeZYW4t/view?usp=sharing
    • https://drive.google.com/file/d/1oLyhM9VDgHERnBVjLErbx4OS3itsRJ3g/view?usp=sharing
    • came across a two locations where behavior was undefined so I dealt with it accordingly
      • the reload button and the refresh command are different ATM so I wired them to trigger the same event. My reasoning was that because the internal button just triggers code refreshes so it could do nothing if the edit=true in the url bar, giving the impression to the user that something is wrong and the page is freezing. So i just made it reload the page. It adds consistency either way
      • second location where behavior is undefined is where the user loads code by adding a query URL, does stuff, and then they load a file from their computer. Then there's both the query URL on the page with edited=true but the code and the file loaded are the local file with the user edits. My decision was to do nothing. This is because on page reload in this situation, it does NOT load the URL code (bc edit=true) and just loads what the user most recently edited which is from the local file. The code clears the URL query param when the user loads a file successfully.

@MarkGillespie
Copy link
Contributor

Looks pretty good to me! Two minor comments:

  • right now you pass different things to the first two parameters of history.pushState in the two places where you call it (i.e. history.pushState({}, document.title, window.location.pathname) and history.pushState(null, "", editedURL)). Even though both options work fine, it would be nice to use the same option both times
  • if I have loaded a file from a url and then I try to load a file from a different url, I have to click Run/Show before it will visualize the new pattern on the right (https://drive.google.com/file/d/1ywZwSaLxjAzcjHfvfWDj1EVJKKO9EXly/view?usp=sharing)

@himalinig
Copy link
Author

Thanks for the comments!
both should be fixed. Here's the testing: https://drive.google.com/file/d/1YKCiOO8315DaT3vPwnQTmTqx1pZ5qlmz/view?usp=sharing

@MarkGillespie
Copy link
Contributor

MarkGillespie commented Feb 16, 2023

Thanks! Unfortunately, I still have some problems in Firefox. It looks like the visualization doesn't refresh after loading new code because of some weird ace bug:

Uncaught TypeError: this.foldWidgets is null
    updateFoldWidgets http://localhost:8081/ace-builds/src-noconflict/ace.js:8983
    _signal http://localhost:8081/ace-builds/src-noconflict/ace.js:3454
    onChange http://localhost:8081/ace-builds/src-noconflict/ace.js:9338
    _signal http://localhost:8081/ace-builds/src-noconflict/ace.js:3454
    applyDelta http://localhost:8081/ace-builds/src-noconflict/ace.js:7379
    insertMergedLines http://localhost:8081/ace-builds/src-noconflict/ace.js:7275
    insert http://localhost:8081/ace-builds/src-noconflict/ace.js:7203
    setValue http://localhost:8081/ace-builds/src-noconflict/ace.js:7109
    setValue http://localhost:8081/ace-builds/src-noconflict/ace.js:12765
    setSource http://localhost:8081/?load=https://knit.work/code/interlock-sheet/swgn2/knitout:462
    readURL http://localhost:8081/?load=https://knit.work/code/interlock-sheet/swgn2/knitout:470
    readURL http://localhost:8081/?load=https://knit.work/code/interlock-sheet/swgn2/knitout:469
    <anonymous> http://localhost:8081/?load=https://knit.work/code/interlock-sheet/swgn2/knitout:614

I'm not sure what's going on. But if everything works for you in Chrome, it seems reasonable to merge the changes with a TODO to investigate what's happening in Firefox

@himalinig
Copy link
Author

I want to fix this issue but I was having some trouble reproducing locally on Firefox. So far no problems with chrome and safari. The error could be racey (?). If everything looks fine in chrome for you as well, let's merge and put the firefox issue in a TODO/issue?

@MarkGillespie
Copy link
Contributor

MarkGillespie commented Feb 16, 2023

I don't have chrome on my laptop, so I tried it on a different computer and everything worked fine there even in firefox. Something weird must be going on with my laptop in particular. Merging sounds good to me. I can try to look into what's happening on my computer later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants