We are 2 versions behind, so here's a patch that updates the fullcalendar library files (js and css).

For reference: http://arshaw.com/fullcalendar/download/

Perhaps instead of including the fullcalendar library files in this module, we should make it a manual dependency, similar to wysiwyg and other modules that require 3rd-party libraries. Thoughts?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Version: 6.x-1.0-beta1 » 6.x-1.2-beta5
Status: Needs review » Postponed

Probably a good idea, but I'm going to postpone this for now. 1) Because it conflicts with #906112: handle drag and drop: creating events, rescheduling events, and 2) because I want to check on the current best practice for including 3rd party libraries.

geerlingguy’s picture

I vote for using Libraries API, and instructing users to download fullcalendar.js and put it in sites/all/libraries.

jwhat’s picture

Yes, I agree.

tim.plunkett’s picture

Title: Update fullcalendar library » Require Libraries API
Version: 6.x-1.2-beta5 » 6.x-1.x-dev
Status: Postponed » Active

We can't have dual licensed JS in the module itself, using Libraries API is a good idea.

tim.plunkett’s picture

Priority: Normal » Major
DamienMcKenna’s picture

Please see how jQuery_UI handled the same issue: #489140: Libraries API

DamienMcKenna’s picture

Quick summary of #489140 for those who don't want to read the full thing:

  • Don't require use of the Libraries API module.
  • Check for the presence of the required JS file in "sites/all/libraries" (default location when using the Libraries API module).
  • Then check for presence of the required JS file via the Libraries API if it exists.

In short, you don't need an extra module just to standardize on using "sites/all/libraries" as the standard 3rd party file location.

jwhat’s picture

Status: Active » Needs review
FileSize
13.67 KB

Thanks to DamienMcKenna for the recommended solution. I implemented it exactly as he suggested: first check sites/all/libraries/fullcalendar, then check via Libraries API (if the module exists, of course), then displaying an error msg if the library is missing.

The fullcalendar.js file needs to be deleted now but I'm not sure how to note that in the patch.

tim.plunkett’s picture

This breaks #910510: Add support for event class / assigning colors to events. Which I should have seen coming.
I'm half tempted to file a patch (pull request?) against FullCalendar itself, suggesting they split out the default styling of events to another stylesheet. Because otherwise, it is overly difficult to override.

Thoughts?

geerlingguy’s picture

I support that idea; It would be great if we could have a separate stylesheet with only the bare minimum of tweaks to make sure FC works in Drupal.

jwhat’s picture

Couldn't we just put that custom CSS into our own fullcalendar.css file and override it like html .fc-event-default etc? If you'd like I can roll that includes this.

jwhat’s picture

I meant, I could roll a patch...

jwhat’s picture

FileSize
14.08 KB

Here's a patch that contains the few customizations that were previously added to the 3rd-party fullcalendar.css... now added to our custom fullcalendar.css.

tim.plunkett’s picture

Currently I can use theme_fullcalendar_classname() to remove .fc-event-default, and add my own class, and the color can change.
Using the official fullcalendar.css, this is not the case. In CSS I would need to follow their example of overriding colors.

Compare this

.myclass {
  background-color: #ff0000;
}

to this

.myclass,
.fc-agenda .myclass .fc-event-time,
.myclass a {
  background-color: #ff0000;
  border-color: none;
  color: inherit;
}

Now consider that I'm using theme_fullcalendar_classname() to assign a class for each taxonomy term on my site. The amount of extra CSS gets out of hand.

tim.plunkett’s picture

FileSize
10.81 KB

This removes the .js and .css file, moves our specific code into fullcalendar.custom.css, and adds drush integration to facilitate downloading the plugin.

tim.plunkett’s picture

The library/version checks and drush integration borrows heavily from colorbox, major props to them.

geerlingguy’s picture

Status: Needs review » Reviewed & tested by the community

Just tested on my install, and everything works fine - I had to move the entire contents of the 'fullcalendar' directory into /sites/all/libraries/fullcalendar, of course. Once I did that, the styling and js file for fullcalendar (1.4.9 or later) worked great.

Great work!

tim.plunkett’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
aspilicious’s picture

FileSize
11.75 KB

This is a starting patch...
I also edited some comments/made a comment framework cause doxygen state of this module is prety bad :)
Btw you should replace all the occurences of colorbox by fullcalendar (docs) in the 6.X branch :)

I didn't test it, it's just a strate port without the jquery stuff.

aspilicious’s picture

Status: Patch (to be ported) » Needs review
tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I think I only missed two references to Colorbox, I couldn't do a straight replace since we actually reference Colorbox.
I'll take a look at this soon I hope.

What do you think about doing a doxygen patch for D6? Split that off into another issue for both branches?

aspilicious’s picture

Yeah, best to split things.

This patch:
-----------
I removed all the array stuff because we only have one dependency in D7.
But I now realize we maybe will have multiple dependencies in the future.
So maybe I should put those arrays back.

tim.plunkett’s picture

Status: Needs review » Patch (to be ported)

That last comment reads as though you intended to attach a patch.
Also, I would put the arrays back, it can't hurt to leave open for further dependencies.

aspilicious’s picture

I don't have time to patch a lot these days...
Srry for that, so if someone else wants to dit it. Go for it.

aspilicious’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.66 KB

Ok ported. I'm going to test this. Post a link when I'm done installing :)

aspilicious’s picture

I don't think I ported this well.
First error message is just because I didn't had the library, is it normal to get such a message?

When looking at the view I get the second error.

aspilicious’s picture

Both error messages are gone now.
Something with dates getting erased when saving (or something like that).
Apparantly something goes banan than cause the module sees the empty field and returns false and goes nuts.

So... almost done testing...
I don't get any good output yet.
Only this:

Test Event
Fri, 07/01/2011 - 08:45 to Fri, 07/01/2011 - 20:11

So it looks like js isn't loaded. Chrome development tools say it is actually loaded. BUT the js from the library hard code months and days. Could this conflict with my dutch installation?

tim.plunkett’s picture

The new hook_library() is awesome!
Revamped to use that instead of hook_init() (which apparently doesn't play well with aggregation).

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Good to go :). Works perfect.

geerlingguy’s picture

Thanks for doing the reviews. Getting a good 7.x branch going will be a tremendous help!

tim.plunkett’s picture

Status: Reviewed & tested by the community » Fixed
aspilicious’s picture

Cleanup of the issue queue!

aspilicious’s picture

Status: Fixed » Closed (fixed)