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

Feature/autosave editor #1633

Merged
merged 36 commits into from Feb 28, 2019

Conversation

@aspittel
Copy link
Contributor

commented Jan 23, 2019

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Add base functionality for autosaving drafts

Related Tickets & Documents

#442 (comment)

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

  • docs.dev.to
  • readme
  • no documentation needed
aspittel added 7 commits Jan 9, 2019
@rhymes

This comment has been minimized.

Copy link
Collaborator

commented Jan 23, 2019

This PR makes me happy 🧨😂

aspittel and others added 6 commits Feb 4, 2019
@benhalpern
Copy link
Collaborator

left a comment

This is close and this implementation is going to rock. Before it can be merged, there are a couple needed changes I didn't notice before.

Currently the session is stored on before unload and shows this dialog:

But it does not "unload" if instead of refreshing I click away from the page because technically we're not changing pages. The dialog also says something to the effect of "changes may not be saved" depending on browsers, and with this functionality I'd say that's not necessary.

Instead of committing to storage on unload, I think it might be best to make this right on every change (possibly asynchronously if it causes jank or anything).

Does that make sense? So we'd write to sessionStorage every time a change happens.

The last thing is that the current buttons do not line up:

Small adjustment needed. Related to this, there is also a small style jump when the JS loads. This is because we have a server-side shell which is plain-old HTML that is displayed before Preact's first write (as opposed to true DRY server-side rendering) so you may need to fiddle with that to address this issue.

@aspittel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@benhalpern these changes should address all those issues! I did move over to local storage so that the changes persist over different tabs. I can move back to session storage, but I think that this probably makes more sense.

@benhalpern
Copy link
Collaborator

left a comment

Final change needs:

It looks like comment_template was removed from the schema here and is causing the tests to fail. That's just an annoyance that happens sometime with schema and branching.

Other changes needed:

  • The buttons are still style jumping on load for the edit view. Everything looks good on /new.
  • As I type, the height of the editor is jumping around. I suspect this could do with the setState on typing or something. Since this editor is still in beta, I wouldn't be opposed to shipping this with that issue still being in place (there was an issue of it jumping once on focus previously). So if you can get to the bottom of that, it's great, otherwise I'm 100% that we are good to go if the tests pass and the other small style issue is dealt with.
@aspittel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Fixed the issue on /edit -- turns out I need a better ERB formatting extension!

The jumping is happening on the production version for me too See the issue, looking into it now!

@aspittel aspittel force-pushed the aspittel:feature/autosave-editor branch from 50782c4 to 80d1ad5 Feb 11, 2019

@aspittel aspittel changed the title [WIP] Feature/autosave editor Feature/autosave editor Feb 19, 2019

@pr-triage pr-triage bot added the PR: unreviewed label Feb 19, 2019

benhalpern and others added 2 commits Feb 20, 2019
@Zhao-Andy
Copy link
Collaborator

left a comment

Looks awesome! Felt so great to test everything and have it auto-saved!!

@benhalpern

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2019

This looks fabulous. I'll merge it first thing in the morning.

@benhalpern benhalpern merged commit 704b7bc into thepracticaldev:master Feb 28, 2019

5 of 7 checks passed

codeclimate 5 issues to fix
Details
codeclimate/total-coverage 86% (-0.1% change)
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate/diff-coverage 100% (50% threshold)
Details
deploy/netlify Deploy preview ready!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.