exposure and binding

[yourilima]

with the config to be still in the ui-calendar tag.

that is not a very important thing but it might be nice to have that be in a config binding (as my last commit removed to be compatible with the tests

[yourilima]

the test fixes was to try out the binding with config instead of the ui-calendar. but as i said in the op. not the most important thing. the tests are back to their old state again.

[yourilima]

My last commit adds an interface object which increases readability of the resulting code when using this directive and increases this directives controll over the fullcalendar object.

this would be desirable for the following things.

when the calendar has been binded to myCalendar you can call myCalendar.refetchEvents() instead of mycalendar.fullCalendar('refetchEvents'). This improves readability.

The directive could add behaviour (event calls, cleanups) to the calls.
The directive could add calls that would execute a series of calls that are often called in succession. (fx remove and add a resource.)

by adding it as a value there also exists the possibility for the user to create its own fullcalendar object bind it to the value and have the same controll without a directive being in the way.

function CalendarCtrl($scope,calendar){
  calendar._calendar = $('#mycalendar');
  calendar.init(options);
  calendar.refetchEvents():
}

[glebm]

Do we need the _s?

[yourilima]

Its a proposal so no. But my reason is this:
Since the _calendar is basically an internal property (and _call an internal function) it should not be accessed from outside the framework/directive.
This allows for making a distinction between internal properties and functions and the properties and functions that are part of the api.

If you have a better way/solution please don't hesitate. I have by no means set things in stone.

for the example it could be changed a bit by adding a bind function....
then it would become something like

function CalendarCtrl($scope,calendar){
  calendar.bind($('#mycalendar'));
  calendar.init(options);
  calendar.refetchEvents():
}

and the bind function internally then does something like

function bind(element){
  this._calendar = element;
}

and can manipulate the element before binding if necesary (check if its empty and thus can safely be made to fullcalendar or throw a warning if not empty)

[glebm]

If I am understanding, this only allows 1 calendar per outer scope?
You could make it a service, or keep it as a value and wrap the whole definition in a closure

[glebm]

I am saying it only allows 1, because all directive instances would share the same instance of the .value (and therefore ._calendar), correct me if I'm wrong

[glebm]

If there is more than one calendar than maybe what we need is a factory which provides the right calendar based on the scope? Could you please explain a bit more how this works? Thanks :)

[yourilima]

I think a factory would be a great Idea. I'm trying it out right now. I'll let you know how that works out.

to explain it a bit more.

My understanding of .value was a bit limited and wrong which is corrected now. and I can see the issue that it only has one instance.
The idea is to provide an object (which could be instantiated) that could encapsulate a jQuery fullCalendar element and expose all functionality trough 'real' function calls ( addEventSource(source) as opposed to .fullCalendar('addEventSource',source) )

Thanks for the input. you made me richer :) (in knowledge ;) )

[yourilima]

there we go no more mystic _ :) and a better providing of the Calendar object.

[glebm]

I see. So you were on there right track, but just a bit confused by lack of angularjs documentation (very common).
This is great!

[glebm]

So does this mean calendar attribute is mandatory now?

[yourilima]

I'm not entirely sure it's the lack of documentation or lack of navigational intuitivity in their documentation...

XD sorry clicked the wrong button...

Yes but again I have not found a way to make it optional... would be better optional I think?

[glebm]

refetchEvents is unnecessary now that all data is tracked automatically (there is a version that rerenders precisely the changes here, so for most people accessing fullCalendar directly will not be necessary. It would be great if it is optional.

[glebm]

You can make it optional by using a local variable internally (e.g. var fullCalendarAdapter = new Calendar...) and writing to external scope only when the attribute is present.

[yourilima]

that would work but i see its a real issue in angular itself too?

angular/angular.js#1435

[glebm]

You can use =? or just get rid of = and use $parent and $eval

[glebm]

@joshkurz What is ui-calendar's minimal compatibility target for angularjs?

[yourilima]

yeah good question. since the =? only works from 1.1.4 :(

[yourilima]

made it optional the same way @joshkurz does it in this PR #12

still prefer/hope to use the =? way

[glebm]

Awesome.

[joshkurz]

yeah that's fine we can stay on the head of the angular repo. I dont have a
problem with that at all.

@yourilima this is looking really nice. I really like the calendar factory.
I think we can get rid of the this.test = 'works' line

We will go with this branch for sure. Thanks for proving me wrong.

Ill play with it for a while and let you know if I find any issues. If not
then Ill rebase everything up and merge.

Good Job.

On Thu, Apr 11, 2013 at 7:14 PM, yourilima [email protected] wrote:

made it optional the same way @joshkurz https://github.com/joshkurzdoes it in this PR
#12 #12

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-16267488
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[glebm]

Although I am not sure about the wrapper methods, because that means we have to support an ever-changing API. Lower maintenance overhead if we use it directly, right?

[glebm]

Maybe a thin wrapper, such as fullCalendar = function() { el.fullCalendar.apply(el, arguments) }

[glebm]

Then if we make that the main way to access fullCalendar, we can expose other stuff in that factory too (like subscribing to changes in events)

[joshkurz]

first of all I would like to point out that whats most important here is
the isolated scope as that is whole purpose of this PR.

Further more we are going into finite details when we talk about wrapping
the calendar. I dont think its a bad idea, and I think yes @glebm that we
should utilize a closer to do so, so that we can take over all of the
calendars api at once.

The whole point of the wrapper is to make the code look more readable
correct? so that you dont have to call scope.calendar.fullCalendar()? Maybe
we really dont need it. If thats all we are trying to do.

But for sure we should just focus on the isolated scope related code in
this PR and then branch everything else out to a new PR where the wrapping
of the API can be discussed.

Make sense?

On Thu, Apr 11, 2013 at 7:57 PM, Gleb Mazovetskiy
[email protected]:

Then if we make that the main way to access fullCalendar, we can expose
other stuff in that factory too (like subscribing to changes in events)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-16268979
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[glebm]

Of course

[joshkurz]

ok cool well then Ill rebase in just the code that has to do with the
isolated scope and we can work with everything else on a separate PR as its
not as important really.

On Thu, Apr 11, 2013 at 8:07 PM, Gleb Mazovetskiy
[email protected]:

Of course

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-16269245
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[glebm]

Awesome

[yourilima]

That makes good sense. Got a bit carried away and polluted the PR a bit. sorry about that

[joshkurz]

its ok NP. you are on the right track. Once we get the rebase in with the
isolated scope then you can create a new PR with the Calendar wrapper if
you want.

On Thu, Apr 11, 2013 at 8:18 PM, yourilima [email protected] wrote:

That makes good sense. Got a bit carried away and polluted the PR a bit.
sorry about that

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-16269578
.

Josh Kurz
www.atlantacarlocksmith.com http://www.atlantacarlocksmith.com

[joshkurz]

you dont need to change this because it would check it in tothe head of the repo. What I do is just take the whole calendar model and put it in the calendarDemo.js file while im deving on the calendar. I just comment this line out until im ready to commit.

[joshkurz]

I left a couple of the commits in the merge, so that we can show the differences in the test files and the calendar file.

Thanks again