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
Merged

Conversation

aspittel
Copy link
Contributor

@aspittel aspittel 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

@rhymes
Copy link
Contributor

rhymes commented Jan 23, 2019

This PR makes me happy 🧨😂

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

aspittel 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.

Copy link
Contributor

@benhalpern benhalpern left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

aspittel 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 changed the title [WIP] Feature/autosave editor Feature/autosave editor Feb 19, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Feb 19, 2019
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

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

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 21, 2019
@benhalpern
Copy link
Contributor

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

@benhalpern benhalpern merged commit 704b7bc into forem:master Feb 28, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants