From 1fbd82129c2194468fe196632f2a3da7bcb45a75 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:18:34 -0600 Subject: [PATCH 01/27] docs: clarify setTitle() with function-level doc --- components/portal/main/services.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 8f5b5509a..cda0a48f3 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -64,7 +64,16 @@ define(['angular'], function(angular) { }; /** - * set the frame title using theme + * set the window title + * + * results in title + * NAMES.title | theme.title (in an application), or + * theme.title (in the root portal) + * + * Examples: + * "STAR Time Entry | MyUW" , for an app named "STAR Time Entry" in + * a portal named "MyUW", or + * "MyUW", for uPortal-home in a portal named "MyUW". */ function setTitle() { var frameTitle = ''; From 0a6caac85287df93b515cd49a39c89f5d69ad205 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:22:53 -0600 Subject: [PATCH 02/27] refactor: clarify setTitle() implementation Splits single variable to one for building up the eventual window title and the other for the name of the portal, which may be a subset of the eventual window title. --- components/portal/main/services.js | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index cda0a48f3..0fbc3148c 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -76,17 +76,25 @@ define(['angular'], function(angular) { * "MyUW", for uPortal-home in a portal named "MyUW". */ function setTitle() { - var frameTitle = ''; + var windowTitle = ''; // we finally set the window title to this. + var portalTitle = ''; // the name of the portal if we can determine it + if ($rootScope.portal && $rootScope.portal.theme) { - frameTitle = $rootScope.portal.theme.title; - if (frameTitle !== NAMES.title && !APP_FLAGS.isWeb) { - frameTitle = ' | ' + frameTitle; + // there's an active theme: use it to determine the name of the portal + portalTitle = $rootScope.portal.theme.title; + + if (portalTitle !== NAMES.title && !APP_FLAGS.isWeb) { + // we're setting the title in the context of an app + // within the portal rather than in the context of uPortal-home + windowTitle = NAMES.title + ' | ' + portalTitle; } else { - // since frame title equals the title in NAMES lets not duplicate it - frameTitle = ''; + // titles like "MyUW | MyUW" e.g. would be silly, + // so just use e.g. "MyUW" + windowTitle = NAMES.title; } } - $document[0].title = NAMES.title + frameTitle; + + $document[0].title = windowTitle; } return { From 305e17bb1974a3fe1066a4ac7db8e74a65013fca Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:30:56 -0600 Subject: [PATCH 03/27] fix: simplify setTitle() to handle app with same name as portal case setTitle() doesn't need to consider whether isWeb, all that matters is whether the app title is redundant with the name of the portal from the theme. --- components/portal/main/services.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 0fbc3148c..65e48718f 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -68,12 +68,12 @@ define(['angular'], function(angular) { * * results in title * NAMES.title | theme.title (in an application), or - * theme.title (in the root portal) + * theme.title (if these names are the same, e.g. in the root portal) * * Examples: * "STAR Time Entry | MyUW" , for an app named "STAR Time Entry" in * a portal named "MyUW", or - * "MyUW", for uPortal-home in a portal named "MyUW". + * "MyUW", for a uPortal-home named "MyUW" in a portal named "MyUW". */ function setTitle() { var windowTitle = ''; // we finally set the window title to this. @@ -83,7 +83,7 @@ define(['angular'], function(angular) { // there's an active theme: use it to determine the name of the portal portalTitle = $rootScope.portal.theme.title; - if (portalTitle !== NAMES.title && !APP_FLAGS.isWeb) { + if (portalTitle !== NAMES.title) { // we're setting the title in the context of an app // within the portal rather than in the context of uPortal-home windowTitle = NAMES.title + ' | ' + portalTitle; From 362399ed6559976f23fff3cd0654c9c524e2ebd1 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:34:12 -0600 Subject: [PATCH 04/27] refactor: handle true branch first Arguably since the true branch here is the exception rather than the rule it should be second in the conditional, but seems more important to remove the ! in the condition to reduce cognitive load in understanding the condition. --- components/portal/main/services.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 65e48718f..426df7d0a 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -83,14 +83,14 @@ define(['angular'], function(angular) { // there's an active theme: use it to determine the name of the portal portalTitle = $rootScope.portal.theme.title; - if (portalTitle !== NAMES.title) { - // we're setting the title in the context of an app - // within the portal rather than in the context of uPortal-home - windowTitle = NAMES.title + ' | ' + portalTitle; - } else { + if (portalTitle == NAMES.title) { // titles like "MyUW | MyUW" e.g. would be silly, // so just use e.g. "MyUW" windowTitle = NAMES.title; + } else { + // we're setting the title in the context of an app + // within the portal rather than in the context of uPortal-home + windowTitle = NAMES.title + ' | ' + portalTitle; } } From 57c16aaf7331a20d85f522f36ba3ed9f4717ddbd Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:35:27 -0600 Subject: [PATCH 05/27] docs: clarify inline comment --- components/portal/main/services.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 426df7d0a..722f2c895 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -88,8 +88,8 @@ define(['angular'], function(angular) { // so just use e.g. "MyUW" windowTitle = NAMES.title; } else { - // we're setting the title in the context of an app - // within the portal rather than in the context of uPortal-home + // app title differs from portal title, so include both in + // window title to communicate app-in-context-of-portal. windowTitle = NAMES.title + ' | ' + portalTitle; } } From 4ed98204ad66f2dddca1794979623faf155cb02a Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:38:36 -0600 Subject: [PATCH 06/27] fix: handle edge case where theme lacks title setTitle() doesn't just need there to be an active theme, it needs that theme to have a .title --- components/portal/main/services.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 722f2c895..5ff1997e0 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -79,8 +79,10 @@ define(['angular'], function(angular) { var windowTitle = ''; // we finally set the window title to this. var portalTitle = ''; // the name of the portal if we can determine it - if ($rootScope.portal && $rootScope.portal.theme) { - // there's an active theme: use it to determine the name of the portal + if ($rootScope.portal && $rootScope.portal.theme && + $rootScope.portal.theme.title) { + // there's an active theme with a title. + // consider that title the name of the portal portalTitle = $rootScope.portal.theme.title; if (portalTitle == NAMES.title) { From f7aa6d1dfa987e498ea2dd648c7348ec7e4cf749 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:41:44 -0600 Subject: [PATCH 07/27] fix: handle case where there isn't an active theme with a title Fall back on local name of app as entire window title. --- components/portal/main/services.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 5ff1997e0..639ced779 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -94,6 +94,10 @@ define(['angular'], function(angular) { // window title to communicate app-in-context-of-portal. windowTitle = NAMES.title + ' | ' + portalTitle; } + } else { + // there isn't an active theme with a title + // so just use the name of the app + windowTitle = NAMES.title; } $document[0].title = windowTitle; From 186864bd3d8cd9d636ddb54621f0b7a1597b808f Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 12:43:39 -0600 Subject: [PATCH 08/27] docs: copy edit inline comment Edit for brevity. Prefer "for example" over "e.g.". --- components/portal/main/services.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 639ced779..82de53a86 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -86,8 +86,8 @@ define(['angular'], function(angular) { portalTitle = $rootScope.portal.theme.title; if (portalTitle == NAMES.title) { - // titles like "MyUW | MyUW" e.g. would be silly, - // so just use e.g. "MyUW" + // titles like "MyUW | MyUW" would be silly, + // so just use "MyUW", for example. windowTitle = NAMES.title; } else { // app title differs from portal title, so include both in From f3b8182684ea7c74f0725d8330deee73f97a7d2e Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 14:49:17 -0600 Subject: [PATCH 09/27] refactor: build windowTitle from pieces Makes setTitle() more clearly read as the algorithm it is implementing. --- components/portal/main/services.js | 37 ++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 82de53a86..92e1c74f2 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -77,29 +77,42 @@ define(['angular'], function(angular) { */ function setTitle() { var windowTitle = ''; // we finally set the window title to this. + + // first, gather pieces of the desired window title + var portalTitle = ''; // the name of the portal if we can determine it + var appTitle = ''; // the name of the app if ($rootScope.portal && $rootScope.portal.theme && $rootScope.portal.theme.title) { // there's an active theme with a title. // consider that title the name of the portal portalTitle = $rootScope.portal.theme.title; + } + + appTitle = NAMES.title; - if (portalTitle == NAMES.title) { - // titles like "MyUW | MyUW" would be silly, - // so just use "MyUW", for example. - windowTitle = NAMES.title; - } else { - // app title differs from portal title, so include both in - // window title to communicate app-in-context-of-portal. - windowTitle = NAMES.title + ' | ' + portalTitle; + // assemble the window title from the gathered pieces, + // starting from the end of the window title and working back to the + // beginning. + + // if the portal has a name, end the window title with that + // (if the portal lacks a name, portalTitle is still empty string) + windowTitle = portalTitle; + + // if the app name differs from the portal, prepend it. + // if it's the same name, omit it to avoid silly redundancy like + // "MyUW | MyUW" + if (appTitle !== portalTitle) { + // if the windowTitle already has content, first add a separator + if (windowTitle !== '') { + windowTitle = ' | ' + windowTitle; } - } else { - // there isn't an active theme with a title - // so just use the name of the app - windowTitle = NAMES.title; + windowTitle = appTitle + windowTitle; } + + // finally, use the built up windowTitle $document[0].title = windowTitle; } From e71dcb9d31cac342dfe9aa6a0b58a8a89d8f2d3b Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 14:50:03 -0600 Subject: [PATCH 10/27] feat: support per-page window titles --- components/portal/main/services.js | 32 +++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 92e1c74f2..50a5a8517 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -67,15 +67,29 @@ define(['angular'], function(angular) { * set the window title * * results in title - * NAMES.title | theme.title (in an application), or - * theme.title (if these names are the same, e.g. in the root portal) + * page-title | app-title | portal-title (in an application), or + * page-title | portal-title (if the app has same name as the portal), or + * app-title | portal-title (if page name unknown or redundant) + * portal-title (if page name unknown or redundant and app name redundant) * * Examples: - * "STAR Time Entry | MyUW" , for an app named "STAR Time Entry" in + * "Timesheets | STAR Time Entry | MyUW" , + * for the Timesheets page in an app named "STAR Time Entry" in * a portal named "MyUW", or - * "MyUW", for a uPortal-home named "MyUW" in a portal named "MyUW". + * + * "Notifications | MyUW", + * for the notifications page in an app named "MyUW" in + * a portal named "MyUW". + * + * "STAR Time Entry | MyUW" + * in an app named "STAR Time Entry" in a portal named "MyUW" + * when the page name is unspecified. + * + * "MyUW" + * in an app named "MyUW" in a portal named "MyUW" + * when the page name is unspecified. */ - function setTitle() { + function setTitle(pageTitle) { var windowTitle = ''; // we finally set the window title to this. // first, gather pieces of the desired window title @@ -111,6 +125,14 @@ define(['angular'], function(angular) { windowTitle = appTitle + windowTitle; } + // if there's a page name not redundant with the app name, prepend it. + if (pageTitle && pageTitle !== '' && pageTitle !== appTitle) { + // if the windowTitle already has content, first add a separator + if (windowTitle !== '') { + windowTitle = ' | ' + windowTitle; + } + windowTitle = pageTitle + windowTitle; + } // finally, use the built up windowTitle $document[0].title = windowTitle; From 83e8bde0904184137040045eecc1a2ee2bca593d Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 25 Jan 2018 16:16:49 -0600 Subject: [PATCH 11/27] test(main-service): unit test setTitle --- components/portal/main/main_service_spec.js | 117 ++++++++++++++++++++ 1 file changed, 117 insertions(+) create mode 100644 components/portal/main/main_service_spec.js diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js new file mode 100644 index 000000000..f36eb4ca1 --- /dev/null +++ b/components/portal/main/main_service_spec.js @@ -0,0 +1,117 @@ +/* + * Licensed to Apereo under one or more contributor license + * agreements. See the NOTICE file distributed with this work + * for additional information regarding copyright ownership. + * Apereo licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file + * except in compliance with the License. You may obtain a + * copy of the License at the following location: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +'use strict'; +/* eslint-env node */ +/* global inject */ +define(['angular-mocks', 'portal'], function() { + describe('mainService', function() { + var service; + + var loginSilentUrl; + var httpBackend; + + var $document; + var $scope; + var NAMES; + + beforeEach(function() { + module('portal'); + }); + + beforeEach(inject(function( + _mainService_, _$httpBackend_, + _$document_, _$rootScope_, + SERVICE_LOC, MESSAGES, APP_FLAGS, _NAMES_ + ) { + $scope = _$rootScope_.$new(); + service = _mainService_; + $document = _$document_; + httpBackend = _$httpBackend_; + NAMES = _NAMES_; + loginSilentUrl = APP_FLAGS.loginOnLoad; + + if (loginSilentUrl) { + httpBackend.whenGET(loginSilentUrl) + .respond({'status': 'success', 'username': 'admin'}); + } + + httpBackend.whenGET(SERVICE_LOC.sessionInfo).respond(); + })); + + it('should set title to MyUW' + + 'when MyUW is both portal name and app name ' + + 'and there is no page name', function() { + // setup + NAMES.title = 'MyUW'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(); + + expect($document[0].title).toEqual('MyUW'); + + httpBackend.flush(); + }); + + it('should set title to STAR | MyUW ' + + 'when STAR is app name and MyUW is portal name ' + + 'and there is no page name', function() { + // setup + NAMES.title = 'STAR'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(); + + expect($document[0].title).toEqual('STAR | MyUW'); + + httpBackend.flush(); + }); + + it('should set title to Timesheets | STAR | MyUW ' + + 'when STAR is app name and MyUW is portal name ' + + 'and Timesheets is page name', function() { + // setup + NAMES.title = 'STAR'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle('Timesheets'); + + expect($document[0].title).toEqual('Timesheets | STAR | MyUW'); + + httpBackend.flush(); + }); + + it('should set title to Notifications | MyUW ' + + 'when MyUW is app name and MyUW is portal name ' + + 'and Notifications is page name', function() { + // setup + NAMES.title = 'MyUW'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle('Notifications'); + + expect($document[0].title).toEqual('Notifications | MyUW'); + + httpBackend.flush(); + }); + }); +}); From e2b15a97f6e7406ba9165338e2fc73ee6c859116 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Fri, 26 Jan 2018 11:23:02 -0600 Subject: [PATCH 12/27] fix(set-title): handle null or empty app name edge case --- components/portal/main/main_service_spec.js | 30 +++++++++++++++++++++ components/portal/main/services.js | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index f36eb4ca1..4ffa02720 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -113,5 +113,35 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); + + it('should set title to Notifications | MyUW ' + + 'when app name is null and MyUW is portal name ' + + 'and Notifications is page name', function() { + // setup + NAMES.title = null; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle('Notifications'); + + expect($document[0].title).toEqual('Notifications | MyUW'); + + httpBackend.flush(); + }); + + it('should set title to Notifications | MyUW ' + + 'when app name is empty and MyUW is portal name ' + + 'and Notifications is page name', function() { + // setup + NAMES.title = ''; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle('Notifications'); + + expect($document[0].title).toEqual('Notifications | MyUW'); + + httpBackend.flush(); + }); }); }); diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 50a5a8517..3455e30dc 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -117,7 +117,7 @@ define(['angular'], function(angular) { // if the app name differs from the portal, prepend it. // if it's the same name, omit it to avoid silly redundancy like // "MyUW | MyUW" - if (appTitle !== portalTitle) { + if (appTitle && appTitle !== portalTitle) { // if the windowTitle already has content, first add a separator if (windowTitle !== '') { windowTitle = ' | ' + windowTitle; From ac6eb3a13e6e76660c5ba197ae5f7579ecdc0cfe Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Fri, 26 Jan 2018 15:47:45 -0600 Subject: [PATCH 13/27] refactor: order tests to match examples Order the tests in the mainService setTitle spec to match the order of examples in the setTitle function doc --- components/portal/main/main_service_spec.js | 60 ++++++++++----------- 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index 4ffa02720..b16b141eb 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -54,36 +54,6 @@ define(['angular-mocks', 'portal'], function() { httpBackend.whenGET(SERVICE_LOC.sessionInfo).respond(); })); - it('should set title to MyUW' - + 'when MyUW is both portal name and app name ' - + 'and there is no page name', function() { - // setup - NAMES.title = 'MyUW'; - $scope.portal.theme.title = 'MyUW'; - - // test - service.setTitle(); - - expect($document[0].title).toEqual('MyUW'); - - httpBackend.flush(); - }); - - it('should set title to STAR | MyUW ' - + 'when STAR is app name and MyUW is portal name ' - + 'and there is no page name', function() { - // setup - NAMES.title = 'STAR'; - $scope.portal.theme.title = 'MyUW'; - - // test - service.setTitle(); - - expect($document[0].title).toEqual('STAR | MyUW'); - - httpBackend.flush(); - }); - it('should set title to Timesheets | STAR | MyUW ' + 'when STAR is app name and MyUW is portal name ' + 'and Timesheets is page name', function() { @@ -143,5 +113,35 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); + + it('should set title to STAR | MyUW ' + + 'when STAR is app name and MyUW is portal name ' + + 'and there is no page name', function() { + // setup + NAMES.title = 'STAR'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(); + + expect($document[0].title).toEqual('STAR | MyUW'); + + httpBackend.flush(); + }); + + it('should set title to MyUW' + + 'when MyUW is both portal name and app name ' + + 'and there is no page name', function() { + // setup + NAMES.title = 'MyUW'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(); + + expect($document[0].title).toEqual('MyUW'); + + httpBackend.flush(); + }); }); }); From 3c078f3080dd73abef4de57bc9f148c1600d0ddb Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Fri, 26 Jan 2018 15:53:30 -0600 Subject: [PATCH 14/27] test: describe tests conceptually rather than literally Use test description to document the bigger picture of each test rather than literally how it tests it. --- components/portal/main/main_service_spec.js | 28 ++++++++------------- 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index b16b141eb..d6852ee77 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -54,9 +54,8 @@ define(['angular-mocks', 'portal'], function() { httpBackend.whenGET(SERVICE_LOC.sessionInfo).respond(); })); - it('should set title to Timesheets | STAR | MyUW ' - + 'when STAR is app name and MyUW is portal name ' - + 'and Timesheets is page name', function() { + it('should set title to include page, app, and portal name when ' + + 'all of these are present and non-redundant.', function() { // setup NAMES.title = 'STAR'; $scope.portal.theme.title = 'MyUW'; @@ -69,9 +68,8 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); - it('should set title to Notifications | MyUW ' - + 'when MyUW is app name and MyUW is portal name ' - + 'and Notifications is page name', function() { + it('should set title to omit app name when redundant with portal name', + function() { // setup NAMES.title = 'MyUW'; $scope.portal.theme.title = 'MyUW'; @@ -84,9 +82,7 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); - it('should set title to Notifications | MyUW ' - + 'when app name is null and MyUW is portal name ' - + 'and Notifications is page name', function() { + it('should set title to omit app name when null', function() { // setup NAMES.title = null; $scope.portal.theme.title = 'MyUW'; @@ -99,9 +95,7 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); - it('should set title to Notifications | MyUW ' - + 'when app name is empty and MyUW is portal name ' - + 'and Notifications is page name', function() { + it('should set title to omit app name when it is empty', function() { // setup NAMES.title = ''; $scope.portal.theme.title = 'MyUW'; @@ -114,9 +108,8 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); - it('should set title to STAR | MyUW ' - + 'when STAR is app name and MyUW is portal name ' - + 'and there is no page name', function() { + it('should set title to omit page name when it is not provided', + function() { // setup NAMES.title = 'STAR'; $scope.portal.theme.title = 'MyUW'; @@ -129,9 +122,8 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); - it('should set title to MyUW' - + 'when MyUW is both portal name and app name ' - + 'and there is no page name', function() { + it('should set title to only the portal name when the app name is ' + + 'redundant and the page name is not provided.', function() { // setup NAMES.title = 'MyUW'; $scope.portal.theme.title = 'MyUW'; From 6f3979a51899c301069d553db1da52bb9497ff3c Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Fri, 26 Jan 2018 15:58:16 -0600 Subject: [PATCH 15/27] test: match test cases with documented examples Match more literally what the test cases test with the examples given in the function doc. --- components/portal/main/main_service_spec.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index d6852ee77..4e345e691 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -57,13 +57,14 @@ define(['angular-mocks', 'portal'], function() { it('should set title to include page, app, and portal name when ' + 'all of these are present and non-redundant.', function() { // setup - NAMES.title = 'STAR'; + NAMES.title = 'STAR Time Entry'; $scope.portal.theme.title = 'MyUW'; // test service.setTitle('Timesheets'); - expect($document[0].title).toEqual('Timesheets | STAR | MyUW'); + expect($document[0].title) + .toEqual('Timesheets | STAR Time Entry | MyUW'); httpBackend.flush(); }); @@ -111,13 +112,13 @@ define(['angular-mocks', 'portal'], function() { it('should set title to omit page name when it is not provided', function() { // setup - NAMES.title = 'STAR'; + NAMES.title = 'STAR Time Entry'; $scope.portal.theme.title = 'MyUW'; // test service.setTitle(); - expect($document[0].title).toEqual('STAR | MyUW'); + expect($document[0].title).toEqual('STAR Time Entry | MyUW'); httpBackend.flush(); }); From 61b462284addd98bf76f4ebb604f6f410c6c2f8a Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Fri, 26 Jan 2018 16:32:42 -0600 Subject: [PATCH 16/27] test: cover more setTitle edge cases --- components/portal/main/main_service_spec.js | 85 +++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index 4e345e691..7dd6a2a8a 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -123,6 +123,34 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); + it('should set title to omit page name when it is null', + function() { + // setup + NAMES.title = 'STAR Time Entry'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(null); + + expect($document[0].title).toEqual('STAR Time Entry | MyUW'); + + httpBackend.flush(); + }); + + it('should set title to omit page name when it is empty string', + function() { + // setup + NAMES.title = 'STAR Time Entry'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle(''); + + expect($document[0].title).toEqual('STAR Time Entry | MyUW'); + + httpBackend.flush(); + }); + it('should set title to only the portal name when the app name is ' + 'redundant and the page name is not provided.', function() { // setup @@ -136,5 +164,62 @@ define(['angular-mocks', 'portal'], function() { httpBackend.flush(); }); + + it('should set title to only the portal name when redundant with both ' + + 'the app name and the page name.', function() { + // setup + NAMES.title = 'MyUW'; + $scope.portal.theme.title = 'MyUW'; + + // test + service.setTitle('MyUW'); + + expect($document[0].title).toEqual('MyUW'); + + httpBackend.flush(); + }); + + it('should set title to only the app name when portal name and page ' + + 'name are null', function() { + // setup + NAMES.title = 'STAR Time and Leave'; + $scope.portal.theme.title = null; + + // test + service.setTitle(null); + + expect($document[0].title).toEqual('STAR Time and Leave'); + + httpBackend.flush(); + }); + + it('should set title to only the page name when portal name and app ' + + 'name are null', function() { + // setup + var priorNamesTitle = NAMES.title; + NAMES.title = null; + $scope.portal.theme.title = null; + + // test + service.setTitle('SomePage'); + + expect($document[0].title).toEqual('SomePage'); + + httpBackend.flush(); + NAMES.title = priorNamesTitle; // undo change made by this test + }); + + it('should gracefully handle missing theme.', function() { + // setup + NAMES.title = 'SomeApp'; + $scope.portal.theme = null; + + // test + service.setTitle('SomePage'); + + expect($document[0].title).toEqual('SomePage | SomeApp'); + + httpBackend.flush(); + }); }); }); From 8ff8529e272983fe867d33f1d825329672c7926e Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 09:41:44 -0600 Subject: [PATCH 17/27] refactor: split title computation from title setting Actually touch the document.title (view) from Controller. Compute the desired title (implement the rule) in the Service. --- components/portal/main/controllers.js | 24 +++- components/portal/main/main_service_spec.js | 135 ++++++++++---------- components/portal/main/services.js | 39 ++---- 3 files changed, 101 insertions(+), 97 deletions(-) diff --git a/components/portal/main/controllers.js b/components/portal/main/controllers.js index fcad84980..ed5a47fb1 100644 --- a/components/portal/main/controllers.js +++ b/components/portal/main/controllers.js @@ -33,6 +33,26 @@ define(['angular', 'require'], function(angular, require) { layoutMode: 'list', // other option is 'widgets }; + /** + * Set Document title. + */ + function updateTitle(pageTitle) { + var appTitle = NAMES.title; + + var portalTitle = ''; + if ($rootScope.portal && $rootScope.portal.theme && + $rootScope.portal.theme.title) { + // there's an active theme with a title. + // consider that title the name of the portal + portalTitle = $rootScope.portal.theme.title; + } + + var windowTitle = + mainService.computeWindowTitle(pageTitle, appTitle, portalTitle); + + $document[0].title = windowTitle; + } + // =====functions ====== var init = function() { $scope.$storage = $localStorage.$default(defaults); @@ -44,13 +64,13 @@ define(['angular', 'require'], function(angular, require) { $scope.APP_OPTIONS = APP_OPTIONS; if (NAMES.title) { - mainService.setTitle(); + updateTitle(); } // https://github.com/Gillespie59/eslint-plugin-angular/issues/231 // eslint-disable-next-line angular/on-watch $rootScope.$watch('portal.theme', function(newValue, oldValue) { if (newValue && newValue !== oldValue) { - mainService.setTitle(); + updateTitle(); } }); diff --git a/components/portal/main/main_service_spec.js b/components/portal/main/main_service_spec.js index 7dd6a2a8a..ce3e92b32 100644 --- a/components/portal/main/main_service_spec.js +++ b/components/portal/main/main_service_spec.js @@ -26,24 +26,16 @@ define(['angular-mocks', 'portal'], function() { var loginSilentUrl; var httpBackend; - var $document; - var $scope; - var NAMES; - beforeEach(function() { module('portal'); }); beforeEach(inject(function( _mainService_, _$httpBackend_, - _$document_, _$rootScope_, - SERVICE_LOC, MESSAGES, APP_FLAGS, _NAMES_ + SERVICE_LOC, MESSAGES, APP_FLAGS ) { - $scope = _$rootScope_.$new(); service = _mainService_; - $document = _$document_; httpBackend = _$httpBackend_; - NAMES = _NAMES_; loginSilentUrl = APP_FLAGS.loginOnLoad; if (loginSilentUrl) { @@ -57,13 +49,15 @@ define(['angular-mocks', 'portal'], function() { it('should set title to include page, app, and portal name when ' + 'all of these are present and non-redundant.', function() { // setup - NAMES.title = 'STAR Time Entry'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = 'Timesheets'; + var appTitle = 'STAR Time Entry'; + var portalTitle = 'MyUW'; // test - service.setTitle('Timesheets'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title) + expect(windowTitle) .toEqual('Timesheets | STAR Time Entry | MyUW'); httpBackend.flush(); @@ -72,53 +66,45 @@ define(['angular-mocks', 'portal'], function() { it('should set title to omit app name when redundant with portal name', function() { // setup - NAMES.title = 'MyUW'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = 'Notifications'; + var appTitle = 'MyUW'; + var portalTitle = 'MyUW'; // test - service.setTitle('Notifications'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('Notifications | MyUW'); + expect(windowTitle).toEqual('Notifications | MyUW'); httpBackend.flush(); }); it('should set title to omit app name when null', function() { // setup - NAMES.title = null; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = 'Notifications'; + var appTitle = null; + var portalTitle = 'MyUW'; // test - service.setTitle('Notifications'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('Notifications | MyUW'); + expect(windowTitle).toEqual('Notifications | MyUW'); httpBackend.flush(); }); it('should set title to omit app name when it is empty', function() { // setup - NAMES.title = ''; - $scope.portal.theme.title = 'MyUW'; - - // test - service.setTitle('Notifications'); - - expect($document[0].title).toEqual('Notifications | MyUW'); - - httpBackend.flush(); - }); - - it('should set title to omit page name when it is not provided', - function() { - // setup - NAMES.title = 'STAR Time Entry'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = 'Notifications'; + var appTitle = ''; + var portalTitle = 'MyUW'; // test - service.setTitle(); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('STAR Time Entry | MyUW'); + expect(windowTitle).toEqual('Notifications | MyUW'); httpBackend.flush(); }); @@ -126,13 +112,14 @@ define(['angular-mocks', 'portal'], function() { it('should set title to omit page name when it is null', function() { // setup - NAMES.title = 'STAR Time Entry'; - $scope.portal.theme.title = 'MyUW'; + var appTitle = 'STAR Time Entry'; + var portalTitle = 'MyUW'; // test - service.setTitle(null); + var windowTitle = + service.computeWindowTitle(null, appTitle, portalTitle); - expect($document[0].title).toEqual('STAR Time Entry | MyUW'); + expect(windowTitle).toEqual('STAR Time Entry | MyUW'); httpBackend.flush(); }); @@ -140,13 +127,15 @@ define(['angular-mocks', 'portal'], function() { it('should set title to omit page name when it is empty string', function() { // setup - NAMES.title = 'STAR Time Entry'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = ''; + var appTitle = 'STAR Time Entry'; + var portalTitle = 'MyUW'; // test - service.setTitle(''); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('STAR Time Entry | MyUW'); + expect(windowTitle).toEqual('STAR Time Entry | MyUW'); httpBackend.flush(); }); @@ -154,13 +143,15 @@ define(['angular-mocks', 'portal'], function() { it('should set title to only the portal name when the app name is ' + 'redundant and the page name is not provided.', function() { // setup - NAMES.title = 'MyUW'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = ''; + var appTitle = 'MyUW'; + var portalTitle = 'MyUW'; // test - service.setTitle(); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('MyUW'); + expect(windowTitle).toEqual('MyUW'); httpBackend.flush(); }); @@ -168,13 +159,15 @@ define(['angular-mocks', 'portal'], function() { it('should set title to only the portal name when redundant with both ' + 'the app name and the page name.', function() { // setup - NAMES.title = 'MyUW'; - $scope.portal.theme.title = 'MyUW'; + var pageTitle = 'MyUW'; + var appTitle = 'MyUW'; + var portalTitle = 'MyUW'; // test - service.setTitle('MyUW'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('MyUW'); + expect(windowTitle).toEqual('MyUW'); httpBackend.flush(); }); @@ -182,13 +175,15 @@ define(['angular-mocks', 'portal'], function() { it('should set title to only the app name when portal name and page ' + 'name are null', function() { // setup - NAMES.title = 'STAR Time and Leave'; - $scope.portal.theme.title = null; + var pageTitle = null; + var appTitle = 'STAR Time and Leave'; + var portalTitle = null; // test - service.setTitle(null); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('STAR Time and Leave'); + expect(windowTitle).toEqual('STAR Time and Leave'); httpBackend.flush(); }); @@ -196,28 +191,30 @@ define(['angular-mocks', 'portal'], function() { it('should set title to only the page name when portal name and app ' + 'name are null', function() { // setup - var priorNamesTitle = NAMES.title; - NAMES.title = null; - $scope.portal.theme.title = null; + var pageTitle = 'SomePage'; + var appTitle = null; + var portalTitle = null; // test - service.setTitle('SomePage'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('SomePage'); + expect(windowTitle).toEqual('SomePage'); httpBackend.flush(); - NAMES.title = priorNamesTitle; // undo change made by this test }); it('should gracefully handle missing theme.', function() { // setup - NAMES.title = 'SomeApp'; - $scope.portal.theme = null; + var pageTitle = 'SomePage'; + var appTitle = 'SomeApp'; + var portalTitle = null; // test - service.setTitle('SomePage'); + var windowTitle = + service.computeWindowTitle(pageTitle, appTitle, portalTitle); - expect($document[0].title).toEqual('SomePage | SomeApp'); + expect(windowTitle).toEqual('SomePage | SomeApp'); httpBackend.flush(); }); diff --git a/components/portal/main/services.js b/components/portal/main/services.js index 2e947ae07..1f1e23038 100644 --- a/components/portal/main/services.js +++ b/components/portal/main/services.js @@ -23,11 +23,9 @@ define(['angular'], function(angular) { .factory('mainService', [ '$http', '$log', '$sessionStorage', - 'miscService', 'SERVICE_LOC', 'APP_FLAGS', '$rootScope', 'NAMES', - '$document', + 'miscService', 'SERVICE_LOC', 'APP_FLAGS', 'NAMES', function($http, $log, $sessionStorage, - miscService, SERVICE_LOC, APP_FLAGS, $rootScope, NAMES, - $document) { + miscService, SERVICE_LOC, APP_FLAGS, NAMES) { var prom = $http.get(SERVICE_LOC.sessionInfo, {cache: true}); var userPromise; @@ -74,9 +72,9 @@ define(['angular'], function(angular) { } /** - * set the window title + * Compute the window title * - * results in title + * Returns * page-title | app-title | portal-title (in an application), or * page-title | portal-title (if the app has same name as the portal), or * app-title | portal-title (if page name unknown or redundant) @@ -99,37 +97,26 @@ define(['angular'], function(angular) { * in an app named "MyUW" in a portal named "MyUW" * when the page name is unspecified. */ - function setTitle(pageTitle) { + function computeWindowTitle(pageTitle, appTitle, portalTitle) { var windowTitle = ''; // we finally set the window title to this. - // first, gather pieces of the desired window title - - var portalTitle = ''; // the name of the portal if we can determine it - var appTitle = ''; // the name of the app - - if ($rootScope.portal && $rootScope.portal.theme && - $rootScope.portal.theme.title) { - // there's an active theme with a title. - // consider that title the name of the portal - portalTitle = $rootScope.portal.theme.title; - } - - appTitle = NAMES.title; - // assemble the window title from the gathered pieces, // starting from the end of the window title and working back to the // beginning. // if the portal has a name, end the window title with that // (if the portal lacks a name, portalTitle is still empty string) - windowTitle = portalTitle; + + if (portalTitle) { + windowTitle = portalTitle; + } // if the app name differs from the portal, prepend it. // if it's the same name, omit it to avoid silly redundancy like // "MyUW | MyUW" if (appTitle && appTitle !== portalTitle) { // if the windowTitle already has content, first add a separator - if (windowTitle !== '') { + if (windowTitle) { windowTitle = ' | ' + windowTitle; } windowTitle = appTitle + windowTitle; @@ -144,14 +131,14 @@ define(['angular'], function(angular) { windowTitle = pageTitle + windowTitle; } - // finally, use the built up windowTitle - $document[0].title = windowTitle; + // finally, return the built up windowTitle + return windowTitle; } return { getUser: getUser, isGuest: isGuest, - setTitle: setTitle, + computeWindowTitle: computeWindowTitle, }; }]); }); From e37ba9d75665a816e5a2c414a3908707e2b2e42e Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 10:51:18 -0600 Subject: [PATCH 18/27] feat: app-header directive side effect sets document title Hacky "solution", with a no-content div using evaluation of the value of a made-up attribute to call the AppHeaderOptionsController to set the title. --- components/portal/misc/controllers.js | 22 +++++++++++++++++-- .../portal/misc/partials/app-header.html | 1 + 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/components/portal/misc/controllers.js b/components/portal/misc/controllers.js index a32c1db96..b2d748f8a 100644 --- a/components/portal/misc/controllers.js +++ b/components/portal/misc/controllers.js @@ -74,13 +74,31 @@ define(['angular'], function(angular) { init(); }]) - .controller('AppHeaderOptionsController', ['APP_OPTIONS', - function(APP_OPTIONS) { + .controller('AppHeaderOptionsController', + ['APP_OPTIONS', 'NAMES', 'mainService', '$rootScope', '$document', + function(APP_OPTIONS, NAMES, mainService, $rootScope, $document) { var vm = this; // If an options template is specified, set scope variable if (APP_OPTIONS.optionsTemplateURL) { vm.optionsTemplate = require.toUrl(APP_OPTIONS.optionsTemplateURL); } + + vm.updateTitle = function(pageTitle) { + var appTitle = NAMES.title; + + var portalTitle = ''; + if ($rootScope.portal && $rootScope.portal.theme && + $rootScope.portal.theme.title) { + // there's an active theme with a title. + // consider that title the name of the portal + portalTitle = $rootScope.portal.theme.title; + } + + var windowTitle = + mainService.computeWindowTitle(pageTitle, appTitle, portalTitle); + + $document[0].title = windowTitle; + }; }]); }); diff --git a/components/portal/misc/partials/app-header.html b/components/portal/misc/partials/app-header.html index 50e4037c1..6588deb45 100644 --- a/components/portal/misc/partials/app-header.html +++ b/components/portal/misc/partials/app-header.html @@ -26,6 +26,7 @@

+
From 9d0700201f97c761430e56372f934a91ba39b591 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 12:07:15 -0600 Subject: [PATCH 19/27] test: test that PortalMainController sets title to reflect theme --- .../portal/main/spec/main_controller_spec.js | 34 +++++++++++++++---- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/components/portal/main/spec/main_controller_spec.js b/components/portal/main/spec/main_controller_spec.js index 6b8136e9b..f6a75d737 100644 --- a/components/portal/main/spec/main_controller_spec.js +++ b/components/portal/main/spec/main_controller_spec.js @@ -21,8 +21,12 @@ /* global inject */ define(['angular-mocks', 'portal'], function() { describe('PortalMainController', function() { - var scope; + var $scope; var $localStorage; + var $document; + var mainService; + var NAMES; + // eslint-disable-next-line no-unused-vars var controller; @@ -30,21 +34,39 @@ define(['angular-mocks', 'portal'], function() { module('portal'); }); - beforeEach(inject(function($rootScope, $controller, _$localStorage_) { - scope = $rootScope.$new(); + beforeEach(inject(function( + _$rootScope_, $controller, _$localStorage_, _$document_, + _mainService_, _NAMES_) { + $scope = _$rootScope_.$new(); $localStorage = _$localStorage_; + $document = _$document_; + mainService = _mainService_; + NAMES = _NAMES_; + + NAMES.title = 'AppName'; + $scope.portal.theme.title = 'PortalName'; + controller = $controller('PortalMainController', { '$localStorage': $localStorage, - '$scope': scope, + '$scope': $scope, + '$rootScope': _$rootScope_, + '$document': $document, + 'mainService': mainService, + 'NAMES': NAMES, }); })); it('should set storage in scope', function() { - expect(scope.$storage).not.toBeNull(); + expect($scope.$storage).not.toBeNull(); }); it('should have an app name defined', function() { - expect(scope.NAMES.title).not.toBeNull(); + expect($scope.NAMES.title).not.toBeNull(); + }); + + it('should set document title to reflect portal name in theme', + function() { + expect($document[0].title).toEqual('AppName | PortalName'); }); }); }); From fe6e427a9244ba22a554cb788ac30107dda15cd4 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 12:11:23 -0600 Subject: [PATCH 20/27] test: note problematic specificity of document title setting test --- components/portal/main/spec/main_controller_spec.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/components/portal/main/spec/main_controller_spec.js b/components/portal/main/spec/main_controller_spec.js index f6a75d737..e2b03a175 100644 --- a/components/portal/main/spec/main_controller_spec.js +++ b/components/portal/main/spec/main_controller_spec.js @@ -66,6 +66,16 @@ define(['angular-mocks', 'portal'], function() { it('should set document title to reflect portal name in theme', function() { + // this test is problematic in that it relies on too much + // knowledge of what mainService will tell PortalMainController + // the window title ought to be. It would be better if this tested + // 1. that PortalMainController called mainService with the correct + // arguments, and + // 2. that PortalMainController set the window title to whatever + // mainService told it the title ought to be. + // that is, mock mainService both to monitor arguments + // PortalMainController calls it with and to fake its response to + // see that PortalMainController honors it expect($document[0].title).toEqual('AppName | PortalName'); }); }); From 19fcbb6852d582fc51598a3d3f191a72e963b868 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 12:15:27 -0600 Subject: [PATCH 21/27] docs: JSDocs PortalMainController.updateTitle(pageTitle) --- components/portal/main/controllers.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/components/portal/main/controllers.js b/components/portal/main/controllers.js index ed5a47fb1..2d45ab182 100644 --- a/components/portal/main/controllers.js +++ b/components/portal/main/controllers.js @@ -35,6 +35,9 @@ define(['angular', 'require'], function(angular, require) { /** * Set Document title. + * Asks mainService what the document title ought to be and + * sets the document title to that value. + * @param {string} pageTitle - Optional, name of specific page viewed. */ function updateTitle(pageTitle) { var appTitle = NAMES.title; From 03698b238e4d56ccb2bcff6fb94156de50e8a153 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 12:32:18 -0600 Subject: [PATCH 22/27] test: test AppHeaderOptionsController updateTitle( pageTitle ) --- .../app_header_options_controller_spec.js | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 components/portal/misc/app_header_options_controller_spec.js diff --git a/components/portal/misc/app_header_options_controller_spec.js b/components/portal/misc/app_header_options_controller_spec.js new file mode 100644 index 000000000..cc8be523f --- /dev/null +++ b/components/portal/misc/app_header_options_controller_spec.js @@ -0,0 +1,77 @@ +/* + * Licensed to Apereo under one or more contributor license + * agreements. See the NOTICE file distributed with this work + * for additional information regarding copyright ownership. + * Apereo licenses this file to you under the Apache License, + * Version 2.0 (the "License"); you may not use this file + * except in compliance with the License. You may obtain a + * copy of the License at the following location: + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +'use strict'; +/* eslint-env node */ +/* global inject */ +define(['angular-mocks', 'portal'], function() { + describe('AppHeaderOptionsController', function() { + var $scope; + var $document; + var mainService; + var NAMES; + + var controller; + + beforeEach(function() { + module('portal'); + }); + + beforeEach(inject(function( + _$rootScope_, $controller, _$document_, + _mainService_, _NAMES_) { + $scope = _$rootScope_.$new(); + $document = _$document_; + mainService = _mainService_; + NAMES = _NAMES_; + + NAMES.title = 'AppName'; + $scope.portal.theme.title = 'PortalName'; + + controller = $controller('AppHeaderOptionsController', { + '$rootScope': _$rootScope_, + '$document': $document, + 'mainService': mainService, + 'NAMES': NAMES, + }); + })); + + it('should expose an updateTitle function', + function() { + expect(controller.updateTitle).not.toBeNull(); + }); + + it('should set document title reflecting page title', + function() { + controller.updateTitle('SpecificPageName'); + // this test is problematic in that it relies on too much + // knowledge of what mainService will tell + // AppHeaderOptionsController the window title ought to be. It + // would be better if this tested + // 1. that the controller called mainService with the correct + // arguments, and + // 2. that the controller set the window title to whatever + // mainService told it the title ought to be. + // that is, mock mainService both to monitor arguments + // the controller calls it with and to fake its response to + // see that the controller honors it + expect($document[0].title) + .toEqual('SpecificPageName | AppName | PortalName'); + }); + }); +}); From 4af8ddb89a00f5ac1f655d65961959bedd9ac2bb Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Tue, 30 Jan 2018 12:44:58 -0600 Subject: [PATCH 23/27] docs: note frame-page directive document title setting side-effect --- docs/directives.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/docs/directives.md b/docs/directives.md index e50d82704..ed47c6980 100644 --- a/docs/directives.md +++ b/docs/directives.md @@ -22,12 +22,17 @@ each of your application's main views. ### Params -* **app-title**: The title that will be displayed in the app header +* **app-title**: Will be displayed in the app header and reflected in document +title * **app-icon**: The icon to use as a prefix to the app title. You can use Font Awesome (i.e. "fa-envelope") or Material Icons (i.e. "email"). * **app-show-add-to-home**: If set to true, will include the add-to-home directive and its controller in the app header (used for apps that are part of the portal ecosystem). * **app-fname**: If provided, it will be used in the add-to-home feature. If not, it'll try to use NAMES.fname constant. * **white-background**: A boolean when set to true with give you a white background. +### Side effects + +`frame-page` has the side effect of setting the document title to reflect its `app-title` attribute, by means of its use of `app-header`. + ## Main menu Makes the side navigation menu available. If you have [configured](configuration.md) either the `appMenuTemplateURL` or `appMenuItems` option, you need to include this directive on every page where you want navigation. For this reason, using the frame-page directive for all your pages is recommended. From bffb3a335202f5b506d70c1ab10ea56ae763fe34 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Wed, 31 Jan 2018 06:17:35 -0600 Subject: [PATCH 24/27] docs(changelog): note window title changes --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1260f9a3e..66fa0b4f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,8 +10,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added ++ `mainService` now offers `computeWindowTitle(...)` (#679) ++ `AppHeaderOptionsController` now offers `updateTitle(...)` (#679) + ### Changed ++ `app-header` (and so, `frame-page`) now set Document title (#679) + ### Fixed ### Removed From 8580be1522f83f2691c26b9c415f9e42e61e0601 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Wed, 31 Jan 2018 06:24:44 -0600 Subject: [PATCH 25/27] refactor: rename updateTitle to updateWindowTitle in Controller layer --- CHANGELOG.md | 2 +- components/portal/main/controllers.js | 6 +++--- .../portal/misc/app_header_options_controller_spec.js | 6 +++--- components/portal/misc/controllers.js | 2 +- components/portal/misc/partials/app-header.html | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66fa0b4f9..7418a2cbb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added + `mainService` now offers `computeWindowTitle(...)` (#679) -+ `AppHeaderOptionsController` now offers `updateTitle(...)` (#679) ++ `AppHeaderOptionsController` now offers `updateWindowTitle(...)` (#679) ### Changed diff --git a/components/portal/main/controllers.js b/components/portal/main/controllers.js index 2d45ab182..905b90d82 100644 --- a/components/portal/main/controllers.js +++ b/components/portal/main/controllers.js @@ -39,7 +39,7 @@ define(['angular', 'require'], function(angular, require) { * sets the document title to that value. * @param {string} pageTitle - Optional, name of specific page viewed. */ - function updateTitle(pageTitle) { + function updateWindowTitle(pageTitle) { var appTitle = NAMES.title; var portalTitle = ''; @@ -67,13 +67,13 @@ define(['angular', 'require'], function(angular, require) { $scope.APP_OPTIONS = APP_OPTIONS; if (NAMES.title) { - updateTitle(); + updateWindowTitle(); } // https://github.com/Gillespie59/eslint-plugin-angular/issues/231 // eslint-disable-next-line angular/on-watch $rootScope.$watch('portal.theme', function(newValue, oldValue) { if (newValue && newValue !== oldValue) { - updateTitle(); + updateWindowTitle(); } }); diff --git a/components/portal/misc/app_header_options_controller_spec.js b/components/portal/misc/app_header_options_controller_spec.js index cc8be523f..a8d4b7951 100644 --- a/components/portal/misc/app_header_options_controller_spec.js +++ b/components/portal/misc/app_header_options_controller_spec.js @@ -51,14 +51,14 @@ define(['angular-mocks', 'portal'], function() { }); })); - it('should expose an updateTitle function', + it('should expose an updateWindowTitle function', function() { - expect(controller.updateTitle).not.toBeNull(); + expect(controller.updateWindowTitle).not.toBeNull(); }); it('should set document title reflecting page title', function() { - controller.updateTitle('SpecificPageName'); + controller.updateWindowTitle('SpecificPageName'); // this test is problematic in that it relies on too much // knowledge of what mainService will tell // AppHeaderOptionsController the window title ought to be. It diff --git a/components/portal/misc/controllers.js b/components/portal/misc/controllers.js index b2d748f8a..a83da8d42 100644 --- a/components/portal/misc/controllers.js +++ b/components/portal/misc/controllers.js @@ -84,7 +84,7 @@ define(['angular'], function(angular) { vm.optionsTemplate = require.toUrl(APP_OPTIONS.optionsTemplateURL); } - vm.updateTitle = function(pageTitle) { + vm.updateWindowTitle = function(pageTitle) { var appTitle = NAMES.title; var portalTitle = ''; diff --git a/components/portal/misc/partials/app-header.html b/components/portal/misc/partials/app-header.html index 6588deb45..6462cbb7c 100644 --- a/components/portal/misc/partials/app-header.html +++ b/components/portal/misc/partials/app-header.html @@ -26,7 +26,7 @@

-
+
From ae441b4792142b6cafda143e600441c8577dc308 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Wed, 31 Jan 2018 06:49:36 -0600 Subject: [PATCH 26/27] docs: note window title update side-effect in appHeader source comment --- components/portal/misc/directives.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/components/portal/misc/directives.js b/components/portal/misc/directives.js index ead7293bc..3e1812cf6 100644 --- a/components/portal/misc/directives.js +++ b/components/portal/misc/directives.js @@ -140,7 +140,8 @@ define(['angular', 'require'], function(angular, require) { * Supports 3 attributes: * *
    - *
  1. app-title: displayed in an h1 child element
    2. + *
  2. app-title: displayed in an h1 child element + * and reflected in window title
    3. *
  3. app-icon: the font awesome icon you want (e.g.: fa-google)
    4. *
  4. app-action-link-*; * url: the url you want, if not set action link hides. @@ -157,6 +158,8 @@ define(['angular', 'require'], function(angular, require) { *
* + * Side effect: updates window title to reflect app-title. + * * See ./partials/app-header.html. */ .directive('appHeader', function() { From 95908843cb1c1068a6cbb271ed1125946b470d82 Mon Sep 17 00:00:00 2001 From: Andrew Petro Date: Thu, 1 Feb 2018 11:47:12 -0600 Subject: [PATCH 27/27] docs: use idiomatic JSDoc syntax for optional pageTitle --- components/portal/main/controllers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/portal/main/controllers.js b/components/portal/main/controllers.js index 905b90d82..030b4bdcf 100644 --- a/components/portal/main/controllers.js +++ b/components/portal/main/controllers.js @@ -37,7 +37,7 @@ define(['angular', 'require'], function(angular, require) { * Set Document title. * Asks mainService what the document title ought to be and * sets the document title to that value. - * @param {string} pageTitle - Optional, name of specific page viewed. + * @param {string} [pageTitle] - Name of specific page viewed. */ function updateWindowTitle(pageTitle) { var appTitle = NAMES.title;