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

Clean up Ember router map #5340

Merged
merged 1 commit into from
May 27, 2015
Merged

Clean up Ember router map #5340

merged 1 commit into from
May 27, 2015

Conversation

jaswilli
Copy link
Contributor

  • Switch resources to routes.
  • No longer nest "settings" routes so the router reflects the way the templates are rendered.
  • Remove renderTemplate override from settings routes.
  • Remove unneeded routes, controllers, and views.
  • Adjust users page so that infinite scroll loading of users works and markup remains the same for Zelda styling.


needs: ['feature'],

showGeneral: Ember.computed('session.user.name', function () {

This comment was marked as abuse.

This comment was marked as abuse.

@cobbspur
Copy link
Member

A lot of this looks good, I think it will make another routing task I was struggling on, a lot easier but I noticed you removed the redirect test for the about page and the redirect route. Consequently if I navigate to /ghost/settings/about I now get a 404 error which doesn't seem desirable.

@jaswilli
Copy link
Contributor Author

if I navigate to /ghost/settings/about I now get a 404 error which doesn't seem desirable.

I'm not sure why you'd want a redirect to that route. It's a protected URL inside a single page app, so links inside the app should be updated and nothing external is linking to it.

@cobbspur
Copy link
Member

I'm not sure what you mean by nothing external linking to it, how do you know that, you are guessing I think there. What makes you think I/somebody doesn't have a link to myblog.com/ghost/settings/about...

You can argue whether the about page has enough significance to warrant a redirect but when we moved debug tools to labs we set up a redirect and so for consistency alone I would think we need a redirect here but if other people disagree that's cool with me, how long we keep the redirect is another point to argue perhaps - maybe the debug redirect can go now?

@jaswilli
Copy link
Contributor Author

Covered in the meeting so no need to go into it, except that for the record, I totally did argue against the Debug redirect 😛 , but since it was only a URL (never linked from the menu) it was argued that a redirect made sense, and I was convinced 😄

I'd be up for removing the Debug redirect unless there's third party documentation out there still referring people to it, in which case we may as well keep it...

@cobbspur
Copy link
Member

I would say dump it, it's been in Labs for long enough and is clearly visible in the new nav menu :)

@jaswilli
Copy link
Contributor Author

Updated--now removes the legacy Debug redirect.

No Issue
- Switch resources to routes.
- No longer nest "settings" routes so the router reflects
  the way the templates are rendered.
- Remove renderTemplate override from settings routes.
- Remove unneeded routes, controllers, and views.
- Adjust users page so that infinite scroll loading of users works
  and markup remains the same for Zelda styling.
@novaugust
Copy link
Contributor

This is great. Nice cleanup, and I dig shifting our paradigm to export default all the things directly.

novaugust added a commit that referenced this pull request May 27, 2015
@novaugust novaugust merged commit 78041cd into TryGhost:master May 27, 2015
@ErisDS
Copy link
Member

ErisDS commented May 27, 2015

and I dig shifting our paradigm to export default all the things directly.

We use the export-at-the-end structure across both node and ember at the moment - so the code base is consistent. I agree that it can be a bit verbose, but consistency is king.

There are other ways to do this, which includes declaring the exports inline, which I believe is confusing in larger files, and also declaring first - I know this works in node not sure if it does in ember-land?

Either way - if there is a sense that this should be changed, it should probably be discussed in an issue or on slack and decided upon for certain, and then rolled out across the codebase.

@novaugust
Copy link
Contributor

I thought the changeover had already begun - I've seen a few PRs getting merged in this style without comment.
It seems to be the standard for ES6, and I think is a more straightforward way of doing things in a file where the .extend is really the last line of the file anyways.

I believe putting the export on the declaration improves readability, particularly in files with a lot of code happening in the declaration. Making a variable there begs the question of "Do we do more things to this than just extend it?" forcing people to scroll through a bunch of code just to double check that the file puts out what they think it should anyways. Nice to have that immediately in most all of our files, where nothing else happens. One less cognitive switch / layer of indirection, is what I'm trying to say.

// foo.js Scroll!
var Foo = Bar.extend({
  //50 lines of code
  //..
  //....
});
export default Foo;

// foo.js No scrolling needed!
export default Bar.extend({
  //50 lines of code
  //..
  //....
});

@jaswilli jaswilli deleted the ember-routing branch May 27, 2015 12:23
@jaswilli
Copy link
Contributor Author

There are a few reasons why I've started making this switch when I touch files in the ember app:

  • This is the format that is preferred by ember-cli. If you ember generate controller mycontroller, the code that is generated is export default Ember.Controller.extend....
  • Naming the thing via a var confuses people because of the "old" way in which the Ember resolver used variable names to look up objects.
var PostsPostController = Ember.Controller.extend();

export default PostsPostController;

The above implies that the name of the variable may be important.

  • It's still "export at the bottom" since nothing occurs below that expression. I agree that if a file has a lot of different exports intermingled with non-exported code that we should have an exports block at the bottom of the file.

kevinansfield added a commit to kevinansfield/Ghost that referenced this pull request Oct 6, 2015
no issue
- drops the `var Foo = Ember.Thing.extend({}); export default Foo;` syntax in favour of exporting directly, eg: `export default Ember.Thing.extend({})`
- discussion on this change [here](TryGhost#5340 (comment)) and [here](TryGhost#5694 (diff))
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.

None yet

4 participants