From 50cdc4cf987a49f6100bb410592bbebcec384111 Mon Sep 17 00:00:00 2001 From: Piotr Sarnacki Date: Wed, 5 Jun 2013 11:27:47 +0200 Subject: [PATCH] Move checking for errors to router rather than template Previously we were checking if we should display an error message by adding if statements in a template. This is not the best way to do it, because it clutters a template and makes code harder to follow. In this commit I move rendering error templates to the router. Code for rendering error when there is no builds is not the best way to do it either, but it can be improved when new router changes are merged to Ember's master and a way Ember Data is handling promises is revised and improved. --- assets/scripts/app/controllers/repo.coffee | 1 - assets/scripts/app/routes.coffee | 14 ++- .../app/templates/builds/not_found.hbs | 1 + assets/scripts/app/templates/builds/show.hbs | 106 +++++++++--------- .../scripts/app/templates/repos/not_found.hbs | 3 + assets/scripts/app/templates/repos/show.hbs | 38 +++---- assets/scripts/app/views/repo/show.coffee | 1 + 7 files changed, 84 insertions(+), 80 deletions(-) create mode 100644 assets/scripts/app/templates/builds/not_found.hbs create mode 100644 assets/scripts/app/templates/repos/not_found.hbs diff --git a/assets/scripts/app/controllers/repo.coffee b/assets/scripts/app/controllers/repo.coffee index 552d6fd9..ff9b2b77 100644 --- a/assets/scripts/app/controllers/repo.coffee +++ b/assets/scripts/app/controllers/repo.coffee @@ -3,7 +3,6 @@ Travis.RepoController = Travis.Controller.extend needs: ['repos', 'currentUser'] currentUserBinding: 'controllers.currentUser' - isError: (-> @get('repo.isError') ).property('repo.isError') slug: (-> @get('repo.slug') ).property('repo.slug') isLoading: (-> @get('repo.isLoading') ).property('repo.isLoading') diff --git a/assets/scripts/app/routes.coffee b/assets/scripts/app/routes.coffee index 6fdc38bb..289669d9 100644 --- a/assets/scripts/app/routes.coffee +++ b/assets/scripts/app/routes.coffee @@ -123,11 +123,19 @@ Travis.SetupLastBuild = Ember.Mixin.create setupController: -> @lastBuildDidChange() @controllerFor('repo').addObserver('repo.lastBuild', this, 'lastBuildDidChange') + @repoDidLoad() + @controllerFor('repo').addObserver('repo.isLoaded', this, 'repoDidLoad') deactivate: -> @_super.apply this, arguments @controllerFor('repo').removeObserver('repo.lastBuild', this, 'lastBuildDidChange') + repoDidLoad: -> + # TODO: it would be nicer to do it with promises + repo = @controllerFor('repo').get('repo') + if repo && repo.get('isLoaded') && !repo.get('repo.lastBuild') + @render('builds/not_found', outlet: 'pane', into: 'repo') + lastBuildDidChange: -> build = @controllerFor('repo').get('repo.lastBuild') @controllerFor('build').set('build', build) @@ -238,15 +246,15 @@ Travis.RepoRoute = Ember.Route.extend Travis.DontSetupModelForControllerMixin, repos = Travis.Repo.bySlug(slug) + self = this + observer = -> if repos.get 'isLoaded' repos.removeObserver 'isLoaded', observer proxy.set 'isLoading', false if repos.get('length') == 0 - # isError is also used in DS.Model, but maybe we should use something - # more focused like notFound later - proxy.set 'isError', true + self.render('repos/not_found', outlet: 'main') else proxy.set 'content', repos.objectAt(0) diff --git a/assets/scripts/app/templates/builds/not_found.hbs b/assets/scripts/app/templates/builds/not_found.hbs new file mode 100644 index 00000000..7fbeea2e --- /dev/null +++ b/assets/scripts/app/templates/builds/not_found.hbs @@ -0,0 +1 @@ +There are no builds for this repository. diff --git a/assets/scripts/app/templates/builds/show.hbs b/assets/scripts/app/templates/builds/show.hbs index 5d43ac76..a72bb37c 100644 --- a/assets/scripts/app/templates/builds/show.hbs +++ b/assets/scripts/app/templates/builds/show.hbs @@ -1,66 +1,62 @@ {{#if loading}} Loading {{else}} - {{#if build}} -
-
-
{{t builds.name}}
-
- - {{#if build.id}} - {{#if build.repo.slug}} - {{#linkTo build repo build}}{{build.number}}{{/linkTo}} - {{/if}} +
+
+
{{t builds.name}}
+
+ + {{#if build.id}} + {{#if build.repo.slug}} + {{#linkTo build repo build}}{{build.number}}{{/linkTo}} {{/if}} -
-
{{t builds.state}}
-
{{capitalize build.state}}
-
{{t builds.finished_at}}
-
{{formatTime build.finishedAt}}
-
{{t builds.duration}}
-
{{formatDuration build.duration}}
+ {{/if}} +
+
{{t builds.state}}
+
{{capitalize build.state}}
+
{{t builds.finished_at}}
+
{{formatTime build.finishedAt}}
+
{{t builds.duration}}
+
{{formatDuration build.duration}}
+
+ + {{#with build.commit}} +
+
{{t builds.commit}}
+
{{formatCommit this}}
+ {{#if build.pullRequest}} +
{{t builds.pull_request}}
+
#{{build.pullRequestNumber}} {{build.pullRequestTitle}}
+ {{else}} + {{#if compareUrl}} +
{{t builds.compare}}
+
{{pathFrom compareUrl}}
+ {{/if}} + {{/if}} + {{#if authorName}} +
{{t builds.author}}
+
{{authorName}}
+ {{/if}} + {{#if committerName}} +
{{t builds.committer}}
+
{{committerName}}
+ {{/if}}
+ {{/with}} - {{#with build.commit}} -
-
{{t builds.commit}}
-
{{formatCommit this}}
- {{#if build.pullRequest}} -
{{t builds.pull_request}}
-
#{{build.pullRequestNumber}} {{build.pullRequestTitle}}
- {{else}} - {{#if compareUrl}} -
{{t builds.compare}}
-
{{pathFrom compareUrl}}
- {{/if}} - {{/if}} - {{#if authorName}} -
{{t builds.author}}
-
{{authorName}}
- {{/if}} - {{#if committerName}} -
{{t builds.committer}}
-
{{committerName}}
- {{/if}} -
- {{/with}} +
{{t builds.message}}
+
{{formatMessage build.commit.message}}
-
{{t builds.message}}
-
{{formatMessage build.commit.message}}
+ {{#unless isMatrix}} +
{{t builds.config}}
+
{{formatConfig build.config}}
+ {{/unless}} +
- {{#unless isMatrix}} -
{{t builds.config}}
-
{{formatConfig build.config}}
- {{/unless}} - - - {{#if build.isMatrix}} - {{view Travis.JobsView jobsBinding="build.requiredJobs" required="true"}} - {{view Travis.JobsView jobsBinding="build.allowedFailureJobs"}} - {{else}} - {{view Travis.LogView jobBinding="build.jobs.firstObject"}} - {{/if}} + {{#if build.isMatrix}} + {{view Travis.JobsView jobsBinding="build.requiredJobs" required="true"}} + {{view Travis.JobsView jobsBinding="build.allowedFailureJobs"}} {{else}} - There are no builds for this repository. + {{view Travis.LogView jobBinding="build.jobs.firstObject"}} {{/if}} {{/if}} diff --git a/assets/scripts/app/templates/repos/not_found.hbs b/assets/scripts/app/templates/repos/not_found.hbs new file mode 100644 index 00000000..57d6f053 --- /dev/null +++ b/assets/scripts/app/templates/repos/not_found.hbs @@ -0,0 +1,3 @@ +
+ The repository at {{slug}} was not found. +
diff --git a/assets/scripts/app/templates/repos/show.hbs b/assets/scripts/app/templates/repos/show.hbs index 599c3ddd..152d29f7 100644 --- a/assets/scripts/app/templates/repos/show.hbs +++ b/assets/scripts/app/templates/repos/show.hbs @@ -2,28 +2,24 @@ {{#if view.isEmpty}} {{view Travis.ReposEmptyView}} {{else}} - {{#if isError}} - The repository at {{slug}} was not found. - {{else}} - {{#if repo.isLoaded}} - {{#with repo}} -
-

{{#linkTo "repo" this}}{{slug}}{{/linkTo}}

-
-
- -

{{description}}

- - {{view Travis.RepoShowTabsView}} - {{view Travis.RepoShowToolsView}} - {{/with}} - -
- {{outlet pane}} + {{#if repo.isLoaded}} + {{#with repo}} +
+

{{#linkTo "repo" this}}{{slug}}{{/linkTo}}

+
- {{else}} - Loading - {{/if}} + +

{{description}}

+ + {{view Travis.RepoShowTabsView}} + {{view Travis.RepoShowToolsView}} + {{/with}} + +
+ {{outlet pane}} +
+ {{else}} + Loading {{/if}} {{/if}}
diff --git a/assets/scripts/app/views/repo/show.coffee b/assets/scripts/app/views/repo/show.coffee index ddfdb5ed..fc0b6177 100644 --- a/assets/scripts/app/views/repo/show.coffee +++ b/assets/scripts/app/views/repo/show.coffee @@ -17,6 +17,7 @@ Travis.reopen # TODO: look into fixing it in more general way pane = Ember.get('_outlets.pane') if @get('controller.repo.isLoaded') && @state == 'inDOM' && + @get('controller.repo.lastBuild') && @get('controller.tab') == 'current' && (!pane || pane.state == 'destroyed') view = @get('controller.container').lookup('view:build') view.set('controller', @get('controller.container').lookup('controller:build'))