Skip to content

PSM Step 1#3965

Merged
novaugust merged 1 commit intoTryGhost:masterfrom
halfdan:3936-psm-1
Sep 10, 2014
Merged

PSM Step 1#3965
novaugust merged 1 commit intoTryGhost:masterfrom
halfdan:3936-psm-1

Conversation

@halfdan
Copy link
Copy Markdown
Contributor

@halfdan halfdan commented Sep 4, 2014

closes #3936

  • Implement new PSM
  • Hook up close action
  • Automatically close when view is destroyed

Comment thread core/client/templates/application.hbs Outdated

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@halfdan halfdan force-pushed the 3936-psm-1 branch 3 times, most recently from 4ea2a3b to 5ccae06 Compare September 5, 2014 14:25

This comment was marked as abuse.

Comment thread core/client/mixins/editor-route-base.js Outdated

This comment was marked as abuse.

@halfdan
Copy link
Copy Markdown
Contributor Author

halfdan commented Sep 6, 2014

Alright - gonna say this is ready for review. I haven't touched the tests yet, but will adjust them once getting feedback to make them pass again.

@halfdan halfdan force-pushed the 3936-psm-1 branch 5 times, most recently from 6fc29c5 to 6cd8e7a Compare September 6, 2014 10:46
@halfdan
Copy link
Copy Markdown
Contributor Author

halfdan commented Sep 6, 2014

Tests pass now.

@PaulAdamDavis Would be great if you could check my CSS changes. Had to move the #entry-controls onto the new PSM in order for the .delete button to not show when creating a new post.

@halfdan halfdan changed the title [WIP] PSM Step 1 PSM Step 1 Sep 6, 2014
@PaulAdamDavis
Copy link
Copy Markdown
Member

@halfdan Looks & works just fine to me. But you say "move", looks like you've just removed them though. Is that right?

@halfdan
Copy link
Copy Markdown
Contributor Author

halfdan commented Sep 6, 2014

@PaulAdamDavis Removed the CSS yes - but the id is still used here: halfdan@6cd8e7a#diff-eb8b4e3d326cfd468351e89277b215b6R1. This can definitely be beautified :)

@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Sep 8, 2014

@PaulAdamDavis @halfdan From reading the comments, I'm not 100% sure whether this is ready to merge? Would love to get this in ;)

@PaulAdamDavis
Copy link
Copy Markdown
Member

@ErisDS This PR gets a 👍 from me.

As @halfdan says, "This can definitely be beautified", but I think that'll be part of a larger refactor when the PSM is more complete and we have a better idea of how the code looks when it's more feature-complete.

@novaugust
Copy link
Copy Markdown
Contributor

This still doesn't close when you click off the PSM? Is that no longer an issue?

@JohnONolan
Copy link
Copy Markdown
Member

Doesn't close when clicking outside PSM - should also close on ESC

@halfdan halfdan force-pushed the 3936-psm-1 branch 3 times, most recently from 23e74f8 to 43e836f Compare September 10, 2014 12:54
@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Sep 10, 2014

This is looking pretty good to me now - @JohnONolan @PaulAdamDavis any further comments or can we merge?

@novaugust
Copy link
Copy Markdown
Contributor

Still doesn't have the close when clicking outside the menu functionality, does it?

@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Sep 10, 2014

WFM 😉

If you're using the pr testing command, you need to delete the old branch before running the command again.

@novaugust
Copy link
Copy Markdown
Contributor

Nevermind then, I hadn't taken it for a spin, was just looking for it in the code =)

@novaugust
Copy link
Copy Markdown
Contributor

@halfdan
Copy link
Copy Markdown
Contributor Author

halfdan commented Sep 10, 2014

It does? Try clearing your cache :-)
On 10 Sep 2014 15:38, "Matt Enlow" notifications@github.com wrote:

Still doesn't have the close when clicking outside the menu functionality,
does it?


Reply to this email directly or view it on GitHub
#3965 (comment).

Comment thread core/client/routes/application.js Outdated

This comment was marked as abuse.

@novaugust
Copy link
Copy Markdown
Contributor

Okay, LGTM and I'm fine with it getting merged, but there is one thing I want to point out first, and would love to fix.

This has 3 separate places where the "close post settings menu" functionality is implemented.
Here, here, and here. I think that should be dried up and made the responsibility of one thing only: the PSM view. Everyone else can interact through it. Alternatively, the action can be put on the application route, since that outlet is hooked up by it. The PSM controller can then implement a needs: 'application' to grab the method.

@halfdan halfdan force-pushed the 3936-psm-1 branch 2 times, most recently from b394052 to 81a996c Compare September 10, 2014 14:46
@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Sep 10, 2014

@halfdan what was in the latest update?

@novaugust
Copy link
Copy Markdown
Contributor

He cleaned up the render's controller logic for me. +1 :shipit: needs DRYing further down the road, but i'll append that to the epic (done)

closes TryGhost#3936
- Implement new PSM
- Hook up close action
- Automatically close when view is destroyed
- Close on click and when pressing ESC
@ErisDS ErisDS mentioned this pull request Sep 10, 2014
5 tasks
novaugust added a commit that referenced this pull request Sep 10, 2014
@novaugust novaugust merged commit 5ccbf79 into TryGhost:master Sep 10, 2014
@halfdan halfdan deleted the 3936-psm-1 branch September 10, 2014 18:16
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.

Post Settings Menu 1: Add new with the existing settings

5 participants