Skip to content
This repository has been archived by the owner on Nov 28, 2022. It is now read-only.

Update title of page in editor to current title of post. #1072

Merged
merged 7 commits into from
Jan 21, 2019
Merged

Update title of page in editor to current title of post. #1072

merged 7 commits into from
Jan 21, 2019

Conversation

Neatoro
Copy link
Contributor

@Neatoro Neatoro commented Nov 13, 2018

refs TryGhost/Ghost#10088
The change moved the title information into a function that returns the
correct title or if no title is set for now "Editor" for the editor route. That should help
with multiple Editor tabs open since it now displays the correct title.

Before:
47715178-8bd1d580-dc71-11e8-9aa8-e239736225cc

After:
screenshot 2018-11-13 at 15 43 42

Got some code for us? Awesome 🎊!

Please include a description of your change & check your PR against this list, thanks!

  • Commit message has a short title & issue references
  • Commits are squashed
  • The build will pass (run ember test from the repo root - will be core/client if working from the submodule in Ghost).

Copy link
Collaborator

@kevinansfield kevinansfield left a comment

Choose a reason for hiding this comment

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

Hey @Neatoro 👋 Thanks for looking at this! I think this approach needs a bit of a rethink, we already have a system in place for working with titles that can be used for this case. There are also a few changes needed to make use of the tools Ember provides rather than reaching out for external dependencies.

app/routes/editor.js Outdated Show resolved Hide resolved
app/routes/editor.js Outdated Show resolved Hide resolved
app/routes/editor.js Outdated Show resolved Hide resolved
app/routes/editor.js Outdated Show resolved Hide resolved
app/templates/editor.hbs Outdated Show resolved Hide resolved
@Neatoro
Copy link
Contributor Author

Neatoro commented Nov 13, 2018

@kevinansfield I tried to implement you requested changes, hope they look better now :) If you wonder I forced pushed on my branch to rebase the review comment commits into the same.

@Neatoro
Copy link
Contributor Author

Neatoro commented Nov 13, 2018

Seems like the tests are failing for some reason 🤔 I checked by triggering my own repo against travis and there different tests and a different amount are failing. (https://travis-ci.org/Neatoro/Ghost-Admin/builds/454706025)

Are they unstable or do I miss something here?

@Neatoro
Copy link
Contributor Author

Neatoro commented Nov 14, 2018

I think I found the issue regarding the failing tests. Somehow he has problems with the router. I need to figure out how to fix them hopefully within the next hours.

@coveralls
Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage increased (+0.03%) to 71.335% when pulling 350b911 on Neatoro:master into e73920e on TryGhost:master.

@Neatoro
Copy link
Contributor Author

Neatoro commented Nov 14, 2018

Fixed the failing test. @kevinansfield could you review it again?

Copy link
Contributor Author

@Neatoro Neatoro left a comment

Choose a reason for hiding this comment

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

Implemented the code review followups

Neatoro and others added 7 commits January 21, 2019 10:17
refs #10088
Change moved the title information into a function that returns the
correct title or if no title is set for now "Editor". That should help
with multiple Editor tabs open.
refs #1072
Changed the logic to use the route object of the controller. Afterwards
overwrote the route object in the tests to use a stubbed version.

Added an unit tests that verifies that the updateTitle method is called.
@kevinansfield
Copy link
Collaborator

Thanks @Neatoro. I've pushed changes to your branch so that I can merge 😄

@Neatoro
Copy link
Contributor Author

Neatoro commented Jan 21, 2019

@kevinansfield Thank you!

@kevinansfield kevinansfield merged commit e261b80 into TryGhost:master Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants