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

Client Post #109

Merged
merged 5 commits into from
Jun 4, 2013
Merged

Client Post #109

merged 5 commits into from
Jun 4, 2013

Conversation

tgriesser
Copy link
Contributor

This follows up on the PR for #108 - it adds the functionality required to save a post as either a draft or published, and accounts for the paginated collections on the "content" list page. It doesn't do the date/queue yet, because I wasn't sure how the UI for that would look.

I know I'm going to be talking with John about Backbone a bit tomorrow, but I wanted to put this up here if anyone wanted to pull it down and kick the tires on it a bit.

It is beginning the process of bringing a few small templates to be parsed on the client side, precompiling adding the grunt handlebars and watcher (to watch for updates & recompile the templates) - Run grunt from the command line after you pull and checkout.

Another convention I've started is to add specific classes to any elements targeted by javascript with a js- prefix. This way we're giving a bit of separation between the style and logic, so it's easier to know what's where when changing either.

Finally, it removes the backbone-layout.js file, replacing with a few simpler methods on the View's prototype. I felt that if we weren't using an official lib (no license, attribution, etc) but instead adding conventions of our own, we should keep these things inside the ghost js files for now as opposed to lib. I'd be happy to discuss with @ricardobeat - but the general premise is that I think it simpler if you don't have to name each subview, because if you're doing things correctly in backbone (listening & rendering on events, etc.) you normally don't need to refer to them by name in the parent, and when you do need that, you can just:

this.childItem = this.addSubview(new ChildItem()).render();
this.append(this.childItem.el);

I'd be alright with going back to requiring a name for each subview if there's a strong argument.

@jgable
Copy link
Contributor

jgable commented Jun 2, 2013

I like this code, looks like a really good start to the backbone stuff.

Here is a scenario that I have used named views for before and I'd like to know how you'd handle it. We have a complex modal that has several different subviews for adding/remove users and setting permissions (several of the subviews are shared with other parts of the app that also use them). The whole modal is a kind of form that when the user hits "submit" on the modal view (parent view) acts sort of as a controller that iterates through each subview and gathers the relevant data it needs from it (what users were created, what permissions were selected, what roles were selected) then posts/saves that data to the api.

Would you just keep an assignment to those subviews on your modal view: this.addedGroupsView = this.addSubview(...)? That seems reasonable, just want to make sure.

@tgriesser
Copy link
Contributor Author

I think in that case I might handle it with event listeners on the individual sub-views... something like:

Backbone.trigger('complexForm:save');
this.model.save();

where each added view had:

this.listenTo(Backbone, 'complexForm:save', this.buildModel);

and then built up the model (or models) that need to be saved individually based on that event firing, saving once in the top level view after all listeners are called... I guess if they're shared between different app components that might not work as well, though you could have separate namespace'd listeners. I've found it easier to do that way, because then the view's can be swapped in/out easily and only control themselves and their relation with the model.

Lately, the only times I've been keeping named references to subviews is if I need to perform animation on them or something where knowing about/calling this.namedSubview.$el.something on it directly makes the most sense, or if there's only supposed to be one of a certain view - to existence check, remove/replace with another later etc.

Not having to worry about one more place to name things is nice, but there might be something specific I'm missing about your case where it'd make more sense...

@ricardobeat
Copy link
Contributor

I'd just implemented the router/init for a parallel PR. backbone-layout was pulled from some clients' projects, I haven't gotten around to publishing it yet. not a fan of extending Backbone.View directly, it's a bit opaque. In my mind a Layout (Page? not sure how I decided on naming) is supposed to work as a thin controller for that page, routing events and handling model creation, instead of splattering logic across the router.

the content of starter.js should be inlined as the final <script> block, to allow for pre-loading of documents using server-side rendering.

@tgriesser
Copy link
Contributor Author

Makes sense that we might not want to extend Backbone.View, we could create a base GhostView or something like that. I agree with not putting too much on the router, I guess I was thinking any view should be able to have a sub-view to be able to split things up into smaller pieces anywhere, not just in a top level "layout" - i.e. the bar at the base of the editor page could be split into smaller views for the tags and the publish button piece, so it'd be good to have something like addSubview on the base view that you're extending on anywhere.

I'll typically keep something like starter.js as it is now and just pull in data from an object with pre-loaded data in a variable before it's run, something like this

Saving post as draft, or publishing
Added HBS parser for some client tmpls
Parsing paginated posts
Added grunt watch for hbs parsing on updates
@ErisDS
Copy link
Member

ErisDS commented Jun 3, 2013

The js- prefix is something that I really like. @JohnONolan happy to make this a standard?

@tgriesser
Copy link
Contributor Author

To give proper credit, the js prefix comes from the github styleguide.

@JohnONolan
Copy link
Member

Perfect link - I was unsure, but now I'm sold. Let's just make sure it makes sense whenever used. Always have to think in the mind-set of the 3rd party dev who may hook-into and use/abuse this not in a way we originally intended.

Provided it doesn't turn into a class-soup-mess-fest. I'm 👍

@ricardobeat
Copy link
Contributor

@ErisDS

@ErisDS
Copy link
Member

ErisDS commented Jun 4, 2013

I was waiting yesterday for you guys to finish discussing this - it's some times hard to tell if I should hold or not. Anyway, I've pulled this down and taken it for a spin and found the following bug:

When you edit a draft post, the blue save/draft button doesn't have any text in it.

Also, just FYI, the schedule post stuff is quite a bit further down the roadmap, but we do need to be able to set the published_date and order by this with the newest item first

@ErisDS
Copy link
Member

ErisDS commented Jun 4, 2013

Also... does it make sense to put the hbs-tpl.js file in /js/ rather than in /tpl/. Even if it is strictly JavaScript, it's still a template file really. Plus it's so darned ugly I'd have it live in /tpl/compiled/.That way we wouldn't need special lint rules :)

* master:
  Adding proper copyright info for Ghost Foundation
  Amending pagination test to have a longer timeout until #110 is done
  server half of #83, posts are draft by default, browse shows published by default
  Adding proper copyright info for Ghost Foundation
…t, using Ghost.View rather than modifying Backbone.View's prototype
@tgriesser
Copy link
Contributor Author

@ErisDS moved the hbs-tpl into /tpl/ as you suggested, I didn't see the issue with the blue draft/save button not having text - did you make sure to run grunt from the command line before checking?

Also @ricardobeat - I added Ghost.View to keep the Backbone.View's prototype clean, that way we can add sub-views from within any view (to be able to break things up into small pieces).

ErisDS added a commit that referenced this pull request Jun 4, 2013
@ErisDS ErisDS merged commit 1249691 into TryGhost:master Jun 4, 2013
@tgriesser tgriesser deleted the client-post branch June 4, 2013 13:55
morficus pushed a commit to morficus/Ghost that referenced this pull request Sep 4, 2014
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.

5 participants