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

Auto-save changes #4305

Closed
wants to merge 1 commit into from
Closed

Auto-save changes #4305

wants to merge 1 commit into from

Conversation

jaswilli
Copy link
Contributor

Refs #4259

  • Auto-save new post when title loses focus.
  • If a post has '(Untitled)' for a title, regenerate slugs
    on all title changes--same behavior as a new post that does
    not yet have a slug.

@novaugust
Copy link
Contributor

👍

@jaswilli
Copy link
Contributor Author

Unfortunately the test failures are real. It's the auto-saving on focus-out that's doing it. In the tests, there are a bunch of "go to the editor and then make sure we're at the right URL" assertions. Casper is triggering a focus out on the title and then the route transitions and the URL changes.

Not sure what to do about that other than adapting the tests to be ok with /ghost/editor/ and /ghost/editor/<post id>/.

@JohnONolan
Copy link
Member

One more bug, I think, populating the title field with (untitled) is breaking the focus on the body.

So:

  1. Focus on title
  2. Press Tab
  3. Focus (very briefly) goes to body
  4. Title populates with "(Untitled)"
  5. Focus from body is lost

@ErisDS
Copy link
Member

ErisDS commented Oct 17, 2014

@jaswilli the changes to the tests sound fine ;)

@jaswilli
Copy link
Contributor Author

I'm going to give it a shot. I have a bad feeling that if focus out is always triggered that it's going to cause a bunch of other tests to fail as well. I guess we'll find out.

Refs #4259
- Auto-save new post when title loses focus.
- If a post has '(Untitled)' for a title, regenerate slugs
  on all title changes--same behavior as a new post that does
  not yet have a slug.
- Adjust some functional tests to handle the automatic transition
  from editor/new to editor/editor and the URL changes that go
  along with it.
@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2014

So the problem with the editor losing focus is much bigger than it might first appear.

It's nothing to do with the introduction of autosave, however the change to making autosave happen when the title loses focus is causing this bug to become much, much more apparent than it is in Ghost at present.

Try the following:

  1. start a new post
  2. enter a title right away
  3. Start typing a little in editor
  4. press cmd/ctrl+s whilst you're typing to initialise a save much like autosave.

You'll notice you lose your focus. This happens no-matter-what on the initial save because of the transition from /ghost/editor/ to /ghost/editor/:id. It does not happen on any subsequent save.

This change to triggering that initial save on defocus of the title field makes this really super obvious because the defocus now happens just as you try to focus and start typing, rather than it happening as a result of you pressing cmd/ctrl+s (which is jarring) or pressing the button (if you press the button you defocus anyway).

I don't know what a solution to this might look like - we can't be the first people to have experienced this so perhaps there is a nice way to restore the correct focus after a transition? Alternatively as we now know the initial save will now always happen on defocus of the title we could have the 'edit' form of the editor always load with focus in the editor same as the 'new' one loads with focus in the title area - but I think that might still provide a jarring experience.

The tests are currently highlighting this issue I think - because some of the keys don't get sent to the correct location because of the focus being messed around with.

Is there any way to transition the route without reloading the editor part of the template?

@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2014

By the way - I've been working on this for a few hours and almost have it all straightened out including tests

@jaswilli
Copy link
Contributor Author

Awesome! Do you want me to back out the changes to the tests?

@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2014

So there are a couple of pretty major bugs with the suggested idea of autosave-once on focusOut of the title field - namely that if you type a title and hit save there's a great big horrible error (which I think is why that particular test was failing) and also, if you open the editor and then navigate somewhere else you end up with an (Untitled) draft with nothing in it.

Soooo.... rather than doing autosave-once (or autosave if new) when focusing out from the title, I've moved it to when focusing into the codemirror textarea. So if you hit tab or click to edit, the route transition happens then.

I've also changed the edit route such that when you load it, focus is in the editor. This seems to work ok.

This means that the one case where you won't get an autosave is if you go straight from the title to the tags and add a tag. If you enter a title and/or tags and then try to navigate away you still get the nag-to-save.

As soon as you go to start creating content, you get an autosave, which I think is reasonably nice.

@novaugust
Copy link
Contributor

That all sounds good to me. Just as a note, you can probably just update the if logic right here if you want to keep saving on title focus-out. Just assert that it actually has a title or some other change

if (this.get('isNew') && this.get('isDirty'))

//or

if (this.get('isNew') && this.get('titleScratch')) //the user typed something into the title bar

@ErisDS ErisDS mentioned this pull request Oct 18, 2014
@jaswilli
Copy link
Contributor Author

👍

I was going to propose doing it that way as working off the title was just too problematic.

If you have it sorted feel free to close this and open a new PR. 💃

@ErisDS
Copy link
Member

ErisDS commented Oct 18, 2014

I've got a new PR (#4307) open with your commit + my own. grunt test-functional --target=client/editor_test.js passed for me locally but still fails on travis so I'm going to play with the tests a little more.

@ErisDS ErisDS closed this Oct 18, 2014
ErisDS added a commit to ErisDS/Ghost that referenced this pull request Oct 18, 2014
issue TryGhost#4305, issue TryGhost#4259, issue TryGhost#1413

- change new->edit transitionToRoute to be replaceRoute
- auto focus in the editor on transition to the edit route
- change the one-time autosave to happen on codemirror focusin instead of title focusout
- re-add removed tests, and reorder broken test
@jaswilli jaswilli deleted the issue-4259 branch October 21, 2014 16:35
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.

4 participants