Skip to content

Conversation

@levincem
Copy link

@levincem levincem commented Apr 3, 2019

So here a pull request for the compatibility with Vanilla 2.8. BUT, i've made other changes to your code, added an end date for the event (optional field). And i replaced the month view with FullCalendar.
But i didn't used the "Vanilla way" to insert Js / Css files.. It's directly called from the view, so not very clean ! I even have a little css block in the template, yes, i've done this "quickly" ! I'm not sure if it can help, but in any case, here you go !

levincem added 3 commits April 3, 2019 20:16
Added an end date for the events
Modified the month view to use fullcalendar (must upload fullcalendar by yourself, v4)
…the month view to use fullcalendar (must upload fullcalendar by yourself, v4)

echo '<div class="', $cssClass, '">';
echo $sender->Form->label('Event Date', 'EventCalendarDate');
echo $sender->Form->label('Date de début', 'EventCalendarDate');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing languages inside of a plugin is a bad idea. People with an unsupported language would see that mixture. The Form->label() method is translating its text so if you translate "Event Date" in your language file, that translation will be used here

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, it's not clean, It was not my intention to publish this code.

$year = date('Y');
$yearRange = $year.'-'.($year + 1);
$fields = explode(',', t('EventCalendar.DateOrder', 'month,day,year'));
$fields = explode(',', t('EventCalendar.DateOrder', 'day,month,year'));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a German I also prefer that format, but the other one is - as far as I know - more common in English language and just to be consistent, I have chosen it here and would like to stick with it. It's customizable by using the language file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but i'm not used to develop using Vanilla features and as it was just for one site .. So i made it quick and dirty !

As the title says.. If it can help, it's cool, if not, sorry !

$sender->addDefinition(
'EventCalendarCategoryIDs',
json_encode(c('EventCalendar.CategoryIDs'))
json_encode(unserialize(c('EventCalendar.CategoryIDs')))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vanilla stopped the support for arrays in the config (somethig I still do not understand), but my prefered way now of dealing with such a simple list would be a comma separated list which gets exploded and imploded. Unserializing shouldn't be needed if serialization doesn't happen elsewhere. If it is done, that other place should be corrected

$cssClass .= ' Hidden';
if (!in_array($categoryID, unserialize(c('EventCalendar.CategoryIDs')))) {
$cssClass .= ' hidden';
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vanilla uses upper case css class names. So "Hidden" is correct

return sprintf(
t('EventCalendar.DateMarkup', '<div class="EventCalendarDate">%2$s On %1$s</div>'),
strftime(t('EventCalendar.DateFormat', '%A, %e. %B %Y'), strtotime($eventDate)),
strftime(t('EventCalendar.DateFormat', '%A, %e %B %Y'), strtotime($eventDate)),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above: that is translatable so that source code doesn't need to be changed


$sender->setData('Events', $eventCalendarModel->get("{$year}-{$month}-01", "{$year}-{$month}-{$daysInMonth}"));

$plusUn = $month+1;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixing language in source code is a bad idea. Furthermore there should be a blank before and after the plus sign. Vanilla normally uses speaking variables. Looking at a variable called "plus one" gives no hint about its meaning

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i don't understand the syntax you're using just the line after

$beginDate = date('Y-m-d', $beginDate);
}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superfluous spaces shoud be avoided

var EventCalendarCategoryIDs = gdn.definition('EventCalendarCategoryIDs');
if ( typeof EventCalendarCategoryIDs !== 'undefined' ) {
EventCalendarCategoryIDs = jQuery.parseJSON(EventCalendarCategoryIDs);
EventCalendarCategoryIDs = JSON.parse(EventCalendarCategoryIDs);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, kill jQuery references whereever it is possible!

@R-J
Copy link
Owner

R-J commented Apr 4, 2019

Thanks for your support and welcome to GitHub!

I've added several comments to your code. As it is by now, I will not merge it. I would ask you to split this:
One pull request for a new language file (that is an added feature)
One pull request for making it work with Vanilla 2.8 (that's a fix)
One pull request for the new end date feature

The first two things will be very easy for me to decide if I accept them or not. As I have noted, I would prefer explode/implode for the category IDs in the config.
There are lines with English text in your French translation. Please ensure that they are really needed or directly translate them. Normally it wouldn't make sense to see English text in a French translation file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants