Page 2 of 3

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 7:30 pm
by Don Chambers
Tomorrow will be fine, but you might want to consider a download location other than spartacus for testing and development purposes.

The smarty template plugin_eventcal_cal.tpl has this, which suggests an article format option:

Code: Select all

{if $is_eventcal_articleformat == true}
  <h3 class="serendipity_date">$CONST.PLUGIN_EVENTCAL_TITLE</h3>
{/if} 
I think it would be good to have that option, but it does not appear in the plugin's configuration. If you enable it, you could follow the code example of bulletproof's plugin_staticpage.tpl or bulletproof's plugin_dynamicform.tpl, with code such as:

Code: Select all

{if $plugin_contactform_articleformat}
   <div class="serendipity_Entry_Date">
       <h3 class="serendipity_date">{$plugin_contactform_name}</h3>
       <div class="serendipity_entry">
           <div class="serendipity_entry_body">
{/if}
And, of course, end it with:

Code: Select all

{if $plugin_contactform_articleformat}
            </div>
        </div>
    </div>
{/if}
Something else that I noticed did not work is the option labeled "Static Pagetitle & URL". The default value for this is "Calendar for Events" (probably should have a default value without spaces). I changed this to just "events" but index.php?serendipity[subpage]=events does not work.

I like the ability for "Intro". Rather than using a text field, can you make it like static page pre-content, complete with editor?

I have not yet spent much time looking at the html and css, but I see a LOT of inline styling. I think all that should get kicked to the stylesheet. And why hard code the master table to 770px!important? How about just 100%? I suspect I will have other comments regarding html and css when I take a closer look.

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 8:04 pm
by Don Chambers
Something else that comes to mind - could you set a value for the smarty variable {$staticpage_pagetitle}? I don't really care what it is, just as long as it is not blank... that way, templates can detect that this is not a regular entry view and avoid processing entries.tpl, which will obviously contain serendipity_Entry_Date, etc.

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 9:06 pm
by Timbalu
Oh Don, Garvin will kill me... ;-)
You are absolutly right, thank you!

Now I know, why staticpage_pagetitle is needed...!
I have done all that, even the subpage ...,
but I am not sure about INTRO and the static like page pre-content, complete with editor setting.
Maybe you can dig deeper for this.
Ill send you a link of my last zip to PN in a few minutes.

Regards,
Ian

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 10:48 pm
by Don Chambers
I found the source of your remaining HTML validation problem:

Code: Select all

    <tr>
     <td class="eventcal_helptip"><span class="mono tip"> Single Events </span></td>
     <td class="eventcal_helptip"><span class="multi tip"> Multi Events </span></td>
     <td class="eventcal_helptip"><span class="recm tip"> Monthly Events </span></td>
     <td class="eventcal_helptip"><span class="recw tip"> Weekly Events </span></td>
    <tr>
You didn't close the row, you opened a new one. :wink:

In your latest version of plugin_eventcal_cal.tpl (I didn't check other files), I see this for images, which will point to the current template's /img/ folder:

Code: Select all

<img src="{serendipity_getFile file="img/blank.png"}" alt="blank" width="10" height="1" />
<img src="{serendipity_getFile file="img/blank.png"}" alt="blank" width="1" height="1" border="0" />
Then I see you assigning a filename to a variable, then using that variable as follows:

Code: Select all

{if $plugin_eventcal_cal_a==1}{assign var=plugin_eventcal_cal_a_name value='img/notes-reject.gif'}{/if}
<img src="{$plugin_eventcal_cal_imgpath}{$plugin_eventcal_cal_a_name|@default:"img/notes-add.gif"}"
{if $plugin_eventcal_cal_ap==1}{assign var=plugin_eventcal_cal_ap_name value='img/notes-reject.gif'}{/if}
<img src="{$plugin_eventcal_cal_imgpath}{$plugin_eventcal_cal_ap_name|@default:"img/notes-add.gif"}"
Personally, I would not have bothered with the variable... I would have {if}{else} two <img> based on the condition... might save a performance penny, might not.

Finally, I see this path:

Code: Select all

<img src="{$plugin_eventcal_cal_imgpath}img/notes-delete.gif" alt="notes-delete" />
Why set a smarty variable for the path? The folder /plugins/ cannot be changed anywhere in s9y. You have total control over the plugin folder name, which will always be /serendipity_event_cal/ unless you change it. Another performance penny - just fetch the images through either:

Code: Select all

src="{$serendipityHTTPPath}plugins/serendipity_event_cal/img/blank.png"
I see you changed the main table from width: 770px to width: 100%; min-width:770px!important; Again, I think a specific pixel width should not be made. Many sites simply do not have 770px width content areas.

I like that you made the bbcode modifier an option... though I still question the value of the modifier at all. Why do you like it?

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 11:53 pm
by onli
Don, the path /plugins/ could change. Garvin mentioned that he have a subfolder with some plugins, so you can't rely on that, if I don't oversee the special case that changes that here. Instead of that, many plugins have an option to enter the path or fetch the images through the event-hook (I used both methods in the livecomment-plugin).

Code: Select all

switch($event) {
     case 'external_plugin':
           switch($eventData) {
                   case 'jquery.elastic.js':
                            header('Content-Type: text/javascript');
                            echo file_get_contents(dirname(__FILE__). '/jquery.elastic.js');
                            break;

...

if ($path_defined) {
    echo '<script type="text/javascript" src="' . $path . 'jquery.elastic.js"></script>'. "\n";
} else {
    echo '<script type="text/javascript" src="' . $serendipity['baseURL'] . 'index.php?/plugin/jquery.elastic.js"></script>'. "\n";
}
You did so many good remarks, it would be a really pity if with that earned authority a small glitch would enter the code ;)
sincerely

Re: new event calendar plugin

Posted: Mon Nov 16, 2009 11:56 pm
by Timbalu
Don Chambers wrote:I found the source of your remaining HTML validation problem:

Code: Select all

    <tr>
     <td class="eventcal_helptip"><span class="mono tip"> Single Events </span></td>
     <td class="eventcal_helptip"><span class="multi tip"> Multi Events </span></td>
     <td class="eventcal_helptip"><span class="recm tip"> Monthly Events </span></td>
     <td class="eventcal_helptip"><span class="recw tip"> Weekly Events </span></td>
    <tr>
You didn't close the row, you opened a new one. :wink:
Oh holy sh...! Bravo!
Don Chambers wrote: In your latest version of plugin_eventcal_cal.tpl (I didn't check other files), I see this for images, which will point to the current template's /img/ folder:

Code: Select all

<img src="{serendipity_getFile file="img/blank.png"}" alt="blank" width="10" height="1" />
<img src="{serendipity_getFile file="img/blank.png"}" alt="blank" width="1" height="1" border="0" />
Yeah, that is absolutly correct, while I am using the default blank file.
Don Chambers wrote: Then I see you assigning a filename to a variable, then using that variable as follows:

Code: Select all

{if $plugin_eventcal_cal_a==1}{assign var=plugin_eventcal_cal_a_name value='img/notes-reject.gif'}{/if}
<img src="{$plugin_eventcal_cal_imgpath}{$plugin_eventcal_cal_a_name|@default:"img/notes-add.gif"}"
{if $plugin_eventcal_cal_ap==1}{assign var=plugin_eventcal_cal_ap_name value='img/notes-reject.gif'}{/if}
<img src="{$plugin_eventcal_cal_imgpath}{$plugin_eventcal_cal_ap_name|@default:"img/notes-add.gif"}"
Personally, I would not have bothered with the variable... I would have {if}{else} two <img> based on the condition... might save a performance penny, might not.
Yes, maybe. I wasn't sure about this.
Don Chambers wrote: Finally, I see this path:

Code: Select all

<img src="{$plugin_eventcal_cal_imgpath}img/notes-delete.gif" alt="notes-delete" />
Why set a smarty variable for the path? The folder /plugins/ cannot be changed anywhere in s9y. You have total control over the plugin folder name, which will always be /serendipity_event_cal/ unless you change it. Another performance penny - just fetch the images through either:

Code: Select all

src="{$serendipityHTTPPath}plugins/serendipity_event_cal/img/blank.png"
I see you changed the main table from width: 770px to width: 100%; min-width:770px!important; Again, I think a specific pixel width should not be made. Many sites simply do not have 770px width content areas.
I set min-width to 770, while we got 7 * 16 characters, margins, paddings, etc, which shouldn't break. Its just a number of experience... ;-) In a small browser window, this plugin isn't really fun and should not be used!
Don Chambers wrote: I like that you made the bbcode modifier an option... though I still question the value of the modifier at all. Why do you like it?
If the form sends a new event, it gets added to the unapproved table. I don't want fulltext here, thats why I use truncate. If there is a truncated text:

Code: Select all

I am a event text and here I < b >describe what will happen... 
I need to close the b tag. Thats why I need the modifier. Its only stupid php code, but it has to be a smarty function, you see....?! Now, if you do not use the modifier it is

Code: Select all

I am a event text and here I [b]describe what will happen... 
so its just a matter of beauty and I am a little bit oldfashioned with beauty... :-) !

Regards,
Ian

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 12:47 am
by Don Chambers
@ onli - I was unaware that plugins could be contained in another folder. I do not believe I have ever encountered that. I wonder how that works with spartacus? There is no option to configure a different plugin folder for plugins downloaded via spartacus. Obviously this will need to change in Ian's plugin, which currently hard codes the smarty path as:

Code: Select all

'plugin_eventcal_cal_imgpath'       => $serendipity['serendipityHTTPPath'] . 'plugins/serendipity_event_cal/',
The live comment plugin looks to be a good example... In fact, I think Ian should just duplicate the whole path and image path concept from that plugin, then register the variables with smarty so they can be used in tpl's.

That looks like a great example.

@ Timbalu/Ian - Why not just use smarty's strip tags instead of that modifier? I know it does not handle bbcode, but it should stop a page from breaking in the event of an unclosed tag.

Also be sure to check errors through a validator with the "insert event" form expanded.... there are not many errors, and they all looked pretty simple to solve IMHO.

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 3:14 pm
by Timbalu
Hi Onli, Don,

what would you think?
Is this matching a different plugin folder?

Code: Select all

$pluginpath = pathinfo(dirname(__FILE__));

...assign(
'ipath' => $serendipity['serendipityHTTPPath'] . basename(rtrim($pluginpath['dirname'], '/')) . '/serendipity_event_cal/'
Ian

@Don
I am not so much aversed about the modifier stuff as you, but I think the strip_tags idea sounds like a good compromise... ;-)

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 3:38 pm
by Don Chambers
Post a link to your revised files and I will test when you are ready.

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 4:13 pm
by Timbalu
Please look into PN -happy testing ;-)

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 4:55 pm
by Don Chambers
Looking better! I suggest you perform one final review using a validator with an unapproved event pending.

While not necessary, you could replace all of those {ldelim} and {rdelim} by wrapping your javascript in {literal}{/literal}. I do not think it is technically any better, I just personally find it easier to read the code that way. Do it however you like, I just wanted to mention in case you did not know about {literal}.

I still see this:

Code: Select all

<img src="{serendipity_getFile file="img/blank.png"}"
Which means it will try to find the image in the current template folder. Is there a reason you did not change that to use your plugin path variable?

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 5:13 pm
by Timbalu
Yeah, I just noticed it myself....
Go and fetch the new version, please.

About blank.png:
I'll erased my files and use the default ones.

Ian

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 5:45 pm
by Don Chambers
I downloaded it again... I cannot see any difference in this version.

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 8:33 pm
by Don Chambers
Now I am turning my attention to html and css. Not much, if any, of this will be errors. Much of it is probably MY personal opinion/preference. Please do not feel the need to change something if you prefer it the way it is.

My general reaction is that the html is bloated more than it needs to be. The same can be said for the css - at over 500 lines, it is bigger than some template's main stylesheet. :wink: And this 500+ lines of css loads for every pageview, not just the event calendar.

Here is an example with error messages:

Code: Select all

<div class="serendipity_center eventcal_tpl_error">
    <div class="eventcal_tpl_error_inner">{$plugin_eventcal_error}</div>
</div>
Which comes with this css:

Code: Select all

    .eventcal_tpl_error {
        border: 1px solid #6280A2;
        background-color: #A29D8C;
        color: #906030;
        font-weight: 580;/**/
        padding: 6px;
    }
    .eventcal_tpl_error_inner {
        width: auto;
        border: 1px solid #6280A2;
        background-color: #FF9030;
        padding: 6px;
    }
Why two divs? Why all the css? Most templates have the class serendipity_msg_important, how about (with, or without the additional class of serendipity_center):

Code: Select all

<div class="serendipity_center serendipity_msg_important">{$plugin_eventcal_error}</div>
One less div, no plugin css at all.

Next, debug code and css. This is just from plugin_eventcal_cal.tpl:

Code: Select all

{if $is_eventcal_cal_debug_fdc}
    <div id="eventcal_error_surrounder">
        <div class="error_brand">  function: {$is_eventcal_cal_debug_fdc} </div>
    </div>
{/if}
{if $is_eventcal_cal_debug_fcwe}
    <div id="eventcal_error_surrounder">
        <div class="error_brand">  function: {$is_eventcal_cal_debug_fcwe} </div>
    </div>
{/if}
{if $is_eventcal_cal_debug_fs1}
    <div id="eventcal_error_surrounder">
        <div class="error_brand">  function: {$is_eventcal_cal_debug_fs1} </div>
    </div>
{/if}
{if $is_eventcal_cal_debug_fs2}
    <div id="eventcal_error_surrounder">
        <div class="error_brand">  function: {$is_eventcal_cal_debug_fs2} </div>
    </div>
{/if}
And the css that goes with it:

Code: Select all

    #eventcal_error_surrounder {
        padding: 4px;
        background-color:#999; 
        border: 1px solid #333;
    }
    .error_brand {
        background-color: #000;
        color: #FF3000;
        font-weight: 800;
    }
Are all those debug conditions necessary? Do they really add value? 4 separate variables? As above, also has 2 divs where one do fine. My suggestion would be to again use the serendipity_msg_important class, and let the template provide css.

There are other error and/or debug conditions in other tpls. My preference would be to change all of them to use the existing serendipity_msg_important class as well, and only keep the debug variables and code that is absolutely necessary. Personally, when I debug something, I keep the code only as long as is necessary for debugging, then I remove it. Again, this is my personal preference - change only if you want to.

These error related classes are defined in the stylesheet, but do not appear in any plugin file:

Code: Select all

    .error_table_main {
        color: #333;
        background-color: #F00000; 
        border: 2px solid #F0F0F0;
    }


    .eventcal_error_bundled {
      width: auto;
      border: 2px dashed #FF3300;
        -moz-border-radius: .0em 0 1em 1em;
    }
    .eventcal_errors {
        font-family: verdana,arial,helvetica,sans-serif;
        font-size: 11px;
        font-weight: 500;
        text-align: left;
        color: #333;
    }
Regarding this:

Code: Select all

<div class="serendipity_center eventcal_tpl_message">
    <div class="serendipity_center serendipity_msg_notice">{$plugin_eventcal_cal_admin}</div>
{foreach from=$plugin_eventcal_message item=message}
    <div class="eventcal_tpl_message_inner">{$message}</div>
{/foreach}
</div>
which uses this css:

Code: Select all

    .eventcal_tpl_message {
        border: 1px solid #6280A2;
        background-color: #C6C6DE;
        padding: 6px;
    }
    .eventcal_tpl_message_inner {
        width: auto;
        border: 1px solid #6280A2;
        background-color: #A29D8C;
        color: #F5DEB3; /* Wheat */
        padding: 6px;
    }
I do like that serendipity_msg_notice was used, but I personally think the other classes are too much.

Re: new event calendar plugin

Posted: Tue Nov 17, 2009 10:05 pm
by Don Chambers
I was looking at the english language file... and spotted a couple of things...

First, the UTF-8 version (both English and German) has a constant defined at the end that is not in the non utf-8 version:

Code: Select all

    @define('PLUGIN_EVENT_SPAMBLOCK_CAPTCHAS_USERDESC', 'your very own text describing captchas');
I'm not sure what purpose this constant serves.

I also did some spot checking of other constants. While this is not intended to be a thorough list, the following constants do not appear to be used in any plugin file:

Code: Select all

    @define('PLUGIN_EVENTCAL_SENT', 'Text after message has been sent');
    @define('PLUGIN_EVENTCAL_SENT_HTML', 'Your message has been successfully mailed!');
    @define('PLUGIN_EVENTCAL_ERROR_HTML', 'An error occured while posting the message!');
    @define('PLUGIN_EVENTCAL_ERROR_DATA', 'Name, e-Mail and your message must not be an empty string.');
    
    @define('PLUGIN_EVENTCAL_CAPTCHAWARNING', '');
    @define('PLUGIN_EVENTCAL_FREE_TABLE', 'Hello Admin.<br />Do you want to free the eventcal table?');
    @define('PLUGIN_EVENTCAL_FREE_TABLE_YES', 'Yes!, go on.');
    @define('PLUGIN_EVENTCAL_FREE_TABLE_NO', 'No!, stop immediately.');
    @define('PLUGIN_EVENTCAL_FREE_TABLE_NOW', '<b>Hello Admin.</b> This MySQL-Table shows a <b>AUTO_INCREMENT</b>-Value of <b>%s</b>.<br />The next <b>ID</b> only needed to be <b>%s</b>. Would you like to free the table now?');
    @define('PLUGIN_EVENTCAL_FREE_TABLE_DONE', 'The eventcal table [<b>\n%s</b>] has been deleted and reinstalled.');

    @define('PLUGIN_EVENTCAL_TEXT_CONVERTBOLDUNDERLINE', 'Enclosing asterisks marks text as bold (*word*), underscore are made via _word_.');
    @define('PLUGIN_EVENTCAL_TEXT_CONVERTSMILIES', 'Standard emoticons like :-) and ;-) are converted to images.');

    @define('PLUGIN_EVENTCAL_ERROR_COLOR_START', '<font color=#ff0000> ');
    @define('PLUGIN_EVENTCAL_ERROR_COLOR_END', ' </font>');
You might want to check for other constants that are no longer being used. These were just ones that caught my attention.