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

Session auto-restore #12

Open
wants to merge 7 commits into
base: tokyo-jan-2012
Choose a base branch
from
Open

Session auto-restore #12

wants to merge 7 commits into from

Conversation

toolness
Copy link
Owner

@stenington, can you take a look at this?

Session auto-restore only works if the goggles are activated on the
page within 5 minutes of data loss.

Also, slightly modified lscache to prefix its localStorage keys with
'webxray-' to avoid potential conflicts w/ lscache on the existing page.
This way the user's changes won't get mysteriously applied when they
try to hack a different page on the same site.
Since we're saving the user's work before they navigate away and
restoring it if they come back, the need for the modal dialog is
obviated.
This ensures that even if the localization tests (which load the en locale)
don't complete (or aren't run), other tests aren't broken.
@toolness
Copy link
Owner Author

Once we pull this in and push to production, we should probably follow-up on this hackasaurus google group thread.

return jQuery.locale.get("input:unload-blocked");
cb();

// Since we are saving the user's work before they leave and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this comment should just replace the comment on line 12. Otherwise they contradict each other.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point. Will do that... and actually, we should probably rename this class to something more descriptive than ModalUnloadBlocker, since that's not at all what it's doing anymore. Maybe something like BeforeUnloadManager or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, works for me.

@stenington
Copy link
Contributor

I think there's a valid use case for wanting to throw away the current hack and start fresh. Maybe providing a key command to start over would be useful, rather than forcing people to wait 5 minutes.

@toolness
Copy link
Owner Author

So the way to let them start fresh would be for them to just "undo" until they couldn't undo any more... In Thimble, we provide a little tooltip telling them as much:

But we're not currently telling them anything with the goggles... We could probably show a little transparent message, though.

@stenington
Copy link
Contributor

Yep, although I actually found that confusing in Thimble because it makes "Undo" modal in a weird way. It'll undo one little step in the normal case, but everything in the special restoration case.

Particularly when I'm new to something, the ability to start fresh is something I want to be easy. But, maybe that's a feature request moreso than a reason to hold up this pull.

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.

2 participants