Changes made to the calcontroller.js file for sharing multiple tokens #765

arunikayadav42 · 2018-03-11T06:05:56Z

I was working on the issue to make more than one calendars visible on the same page.Thus different tokens separated by "." were split using "." as the delimiter and each token was accessed individually.

opticod · 2018-03-11T10:33:41Z

@arunikayadav42 TravisCI is failing due to JSHint which is a tool that helps to detect errors and potential problems in your JavaScript code. Try fixing this, and repush again, to ran travis again. You can get more info by clicking each build jobs in travis site.

georgehrke · 2018-03-11T11:42:41Z

js/app/controllers/calcontroller.js

@@ -140,15 +140,20 @@ app.controller('CalController', ['$scope', 'Calendar', 'CalendarService', 'VEven
 				$scope.$apply();
 			});
 		} else {
-			$scope.calendarsPromise = CalendarService.getPublicCalendar(constants.publicSharingToken).then(function(calendar) {
+			var splitted = constants.publicSharingToken.split(".")


Please add a semicolon here :)

georgehrke · 2018-03-11T11:43:26Z

js/app/controllers/calcontroller.js

-			$scope.calendarsPromise = CalendarService.getPublicCalendar(constants.publicSharingToken).then(function(calendar) {
+			var splitted = constants.publicSharingToken.split(".")
+			var i;
+			for(i = 0; i < splitted.length; i++ )


You could also use constants.publicSharingToken.split(".").forEach(token => {...} here :)

georgehrke · 2018-03-11T11:45:33Z

js/app/controllers/calcontroller.js

+			var i;
+			for(i = 0; i < splitted.length; i++ )
+			{
+				$scope.calendarsPromise = CalendarService.getPublicCalendar(splitted[i]).then(function(calendar) {
 				$scope.calendars = [calendar];
 				is.loading = false;
 				// TODO - scope.apply should not be necessary here
 				$scope.$apply();


Please only call is.loading = false and $scope.$apply(); once all calendars are done loading and not for each individually.

… with the for loop made more concise

… concise and the . added.

arunikayadav42 · 2018-03-13T04:41:45Z

@georgehrke I have made the necessary changes, please review my request :)

tcitworld · 2018-03-13T08:34:41Z

js/app/controllers/calcontroller.js

 				is.loading = false;
 				// TODO - scope.apply should not be necessary here
 				$scope.$apply();
-			}).catch((reason) => {
+				}).catch((reason) => {
 				angular.element('#header-right').css('display', 'none');


Seems to be indent issues here. Does your editor understands .editorconfig files (maybe with a plugin) ?

@tcitworld I am using sublime text to edit the code, can you please explain why my pull request can't be merged :)

Install this package : https://packagecontrol.io/packages/EditorConfig

There's a few indenting issues I see (when opening a bracket, add a new line indented).

I have installed the package and restarted the sublime text but still cannot see any changes made in the indentation of the code.

I didn't use Sublime Text for a while, doesn't it shows some errors somehow ?

Don't bother too much for this, I'll fix these kind of typos afterwards.

tcitworld · 2018-03-13T09:46:25Z

js/app/controllers/calcontroller.js

 				angular.element('#header-right').css('display', 'none');
 				angular.element('#emptycontent-container').css('display', 'block');
-			});
+			});		


Remove spaces here

tcitworld · 2018-03-13T09:47:04Z

js/app/controllers/calcontroller.js

@@ -140,15 +140,18 @@ app.controller('CalController', ['$scope', 'Calendar', 'CalendarService', 'VEven
 				$scope.$apply();
 			});
 		} else {
-			$scope.calendarsPromise = CalendarService.getPublicCalendar(constants.publicSharingToken).then(function(calendar) {
+			constants.publicSharingToken.split(".").forEach(token=>


Add spaces before and after token, and I think it should be inside some parenthesis.

tcitworld · 2018-03-13T09:47:44Z

js/app/controllers/calcontroller.js

 			}).catch((reason) => {
 				angular.element('#header-right').css('display', 'none');
 				angular.element('#emptycontent-container').css('display', 'block');
-			});
+			});		


Remove spaces added at the end of the line.

Is it alright now or do I further look into the indentation ,please tell :)

@arunikayadav42 No, its still there. Remove spaces added at the end of the line.

tcitworld · 2018-03-13T09:49:17Z

js/app/controllers/calcontroller.js

+			constants.publicSharingToken.split(".").forEach(token=>
+			{
+				$scope.calendarsPromise = CalendarService.getPublicCalendar(token).then(function(calendar) {
+					$scope.calendars = [calendar];


If I'm not wrong, this will overwrite $scope.calendars with an array of the current calendar only. You need to push it instead, like $scope.calendars.push(calendar);.

arunikayadav42 · 2018-03-14T11:36:29Z

Can I please know if something remains that is preventing from getting this PR merged :)

tcitworld · 2018-03-14T11:38:16Z

See #765 (comment)

arunikayadav42 · 2018-03-14T11:40:13Z

Do you mean the scope.calendars statement needs to be rectified?

arunikayadav42 · 2018-03-17T05:18:40Z

@tcitworld I have already changed the indentation, can you please tell me what further remains to change?

opticod · 2018-03-17T06:03:55Z

@arunikayadav42 No, its still there. Remove spaces added at the end of the line.

georgehrke · 2018-03-17T10:04:46Z

js/app/controllers/calcontroller.js

-				$scope.calendars = [calendar];
+			constants.publicSharingToken.split(".").forEach( (token) => {
+				$scope.calendarsPromise = CalendarService.getPublicCalendar(token).then(function(calendar) {
+					$scope.calendars = [calendar];


This is still wrong. You are always overwriting $scope.calendars with a new array with one item here.
Instead you need to initialize an empty array first and push each calendar into the array individually.

arunikayadav42 · 2018-03-17T11:21:48Z

@georgehrke or @tcitworld please review :)

georgehrke · 2018-03-18T21:13:23Z

js/app/controllers/calcontroller.js

+			constants.publicSharingToken.split(".").forEach( (token) => {
+				$scope.calendarsPromise = CalendarService.getPublicCalendar(token).then(function(calendar) {
+					$scope.calendars.push(calendar);
+			});


I'm sorry, but the indentation is still wrong. This should be indented by one tab.

georgehrke · 2018-03-18T21:18:55Z

js/app/controllers/calcontroller.js

+			constants.publicSharingToken.split(".").forEach( (token) => {
+				$scope.calendarsPromise = CalendarService.getPublicCalendar(token).then(function(calendar) {
+					$scope.calendars.push(calendar);
+			});
 				is.loading = false;
 				// TODO - scope.apply should not be necessary here
 				$scope.$apply();


These two lines are still called for each calendar. Please only call them once all calendars loaded.

To make sure they are called after all are done, you need to create an array with all CalendarService.getPublicCalendar(token) promises and do a Promise.all() on the array.

For more information about Promise functions see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/all

Furthermore the catch is currently called on the forEach function. It has to be called on the Promise instead.

I will look into the same :)

haug-den-lucas · 2018-07-12T09:34:52Z

There's also another problem:
The "Embed" icon on the bottom will only show the embed code for one of the calendars.
This is fixed in: #865

georgehrke · 2018-08-15T18:33:57Z

Stale, closing in favor of #870, which is implementing the same

Changes made to the calcontroller.js file for sharing multiple tokens

df74643

georgehrke requested changes Mar 11, 2018

View reviewed changes

georgehrke added the 2. developing Work in progress label Mar 11, 2018

georgehrke assigned arunikayadav42 Mar 11, 2018

georgehrke mentioned this pull request Mar 11, 2018

Changes made to the calcontroller.js file #764

Closed

arunikayadav42 added 2 commits March 12, 2018 00:40

Changes made to the calcontroller.js file for sharing multiple tokens…

356ea35

… with the for loop made more concise

Changes made to the calcontroller.js file with the for loop made more…

b9accfc

… concise and the . added.

tcitworld reviewed Mar 13, 2018

View reviewed changes

indentation made proper in the calcontroller.js file

630a584

tcitworld reviewed Mar 13, 2018

View reviewed changes

further indentation made in the calcontroller.js file

7547102

Extra spaces at the end of the line removed

2f18a6a

georgehrke requested changes Mar 17, 2018

View reviewed changes

push function added to append multiple calendars

c40daa2

georgehrke mentioned this pull request Mar 18, 2018

New design for embedding shared calendar #741

Closed

georgehrke requested changes Mar 18, 2018

View reviewed changes

haug-den-lucas mentioned this pull request Jul 15, 2018

Feature request: Share multiple calendars in one link #708

Closed

georgehrke closed this Aug 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes made to the calcontroller.js file for sharing multiple tokens #765

Changes made to the calcontroller.js file for sharing multiple tokens #765

arunikayadav42 commented Mar 11, 2018

opticod commented Mar 11, 2018

georgehrke Mar 11, 2018

georgehrke Mar 11, 2018

georgehrke Mar 11, 2018

arunikayadav42 commented Mar 13, 2018

tcitworld Mar 13, 2018

arunikayadav42 Mar 13, 2018

tcitworld Mar 13, 2018

arunikayadav42 Mar 13, 2018

tcitworld Mar 13, 2018

tcitworld Mar 13, 2018

tcitworld Mar 13, 2018

tcitworld Mar 13, 2018

arunikayadav42 Mar 13, 2018

opticod Mar 17, 2018

tcitworld Mar 13, 2018 •

edited

Loading

arunikayadav42 commented Mar 14, 2018

tcitworld commented Mar 14, 2018

arunikayadav42 commented Mar 14, 2018

arunikayadav42 commented Mar 17, 2018

opticod commented Mar 17, 2018

georgehrke Mar 17, 2018

arunikayadav42 commented Mar 17, 2018

georgehrke Mar 18, 2018

georgehrke Mar 18, 2018

georgehrke Mar 18, 2018

arunikayadav42 Mar 22, 2018

haug-den-lucas commented Jul 12, 2018 •

edited

Loading

georgehrke commented Aug 15, 2018

Changes made to the calcontroller.js file for sharing multiple tokens #765

Changes made to the calcontroller.js file for sharing multiple tokens #765

Conversation

arunikayadav42 commented Mar 11, 2018

opticod commented Mar 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunikayadav42 commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcitworld Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

arunikayadav42 commented Mar 14, 2018

tcitworld commented Mar 14, 2018

arunikayadav42 commented Mar 14, 2018

arunikayadav42 commented Mar 17, 2018

opticod commented Mar 17, 2018

Choose a reason for hiding this comment

arunikayadav42 commented Mar 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haug-den-lucas commented Jul 12, 2018 • edited Loading

georgehrke commented Aug 15, 2018

tcitworld Mar 13, 2018 •

edited

Loading

haug-den-lucas commented Jul 12, 2018 •

edited

Loading