Update of Sidebar Logo Plugin or should I fork?

Creating and modifying plugins.
Post Reply
khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Fri Nov 15, 2013 4:13 pm

Hello everyone,
a few weeks ago I finally decided to take some time to get my blog working. Up to then, it was a half dead website with s9y and coffee cup. Now after doing all the theming, I wanted to have a about box as many blogs have. Nothing more than an image with some text next to the entries. I figured, Sidebar Logo can do it for me. However, this plugin is really old and most of all produces wrong HTML.

Therefore I decided to fix it. I added the alt-property to image and moved the styles into the theme style sheet. However, during the process of doing so, I discovered that it would be much cleaner to rely on the default styles provided by themes and that most features this plugin provides are difficult to understand and to layout. Therefore, I decided to strip it down to a logo and some text. All what could be done before is possible using HTML within this text.

Since I benefit so much from s9y, I'd like to give back this minor contribution. This leaves me with some questions: By simplifying the plugin, I really changed its functionality. I do believe that all what has been possible is possible still. Yet I want to know if this is considered an update or is it a new plugin? Personally I'd prefer to consider it an update since the functionality is very similar and the original plugin is really old and produces incorrect HTML. I'D like to rename it to something like "About box" to be more descriptive. Is there any policy within the community how to handle this?

Regards,
Oliver

User avatar
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by Timbalu » Sat Nov 16, 2013 11:38 am

Thanks.
I'd say, if it is a contribution, the best way to do is creating a pull request, to change and update the original. We do not want to have multi plugins of the same. This indeed is a Serendipity policy.

I don't see where the origin could produce wrong html or other bad things, so a (GitHub) pull request would help, to see clearly any relevant changes. If those changes are too individual, we just deny to accept this request. But you can still make use of your changed or new (by cloning and renaming) plugin.

Or you can use the serendipity_html_nugget_plugin, which is exactly this, isn't it?
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian

khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Tue Nov 19, 2013 11:31 pm

Thanks for your feedback. I created a pull request as you suggested. I tried to explain the reason for my changes, hoping that they find approval.

The current version of the plugin as well as the original use a plain edit area for description text. Is it possible to use the same WYSIWG editor used for creating entries within a plugin? I think that this would further improve the plugin.

Regards,
Oliver

User avatar
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by Timbalu » Wed Nov 20, 2013 10:42 am

khor1701 wrote:Is it possible to use the same WYSIWG editor used for creating entries within a plugin? I think that this would further improve the plugin.
Yes. Change $propbag->add('type', 'text') to $propbag->add('type', 'html');

Another Serendipity policy is the keep compatibility. Does your commit (PR) follow this rule too?

I have never used this plugin, but could you just tell me why you did not use the serendipity_html_nugget_plugin for this? What is the main difference?
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian

khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Sat Nov 23, 2013 3:06 pm

Thanks for your reply. Thanks for the help with the HTML editor. It did not work for me. But due to some changes I made, it is not this important any more. I will explain the changes lateron.

To be straight and honest: My PR does not preserve compatability. This is why I was starting this thread. However, I have a newer version that almost will. But before coming to that, let me describe the current changes and their reasons:

Initial problem was that the <style> tag was generate somewhere in the <body>. This does not comform to HTML specification. The HTML validator marks this as an error although most browsers seem to accept it. Since I liked to have HTML compliant code, this problem got me started to work on the plugin. Additionally, the logo does not provide an alt-attribute. This as well is required by HTML. And whoever worked with visually impaired persons knows that this attribute is truly helpfulp. Adding the alt-attribute was easy. It just requires an extra configuration option. Although sidebarlogo already has a lot of options another option seemed to be acceptable for me. Solving the stylesheet problem was a different story.

The first approach I took was simply adding the style definitions from the plugins in to the css of my theme and use the classes inside the plugin. This does work for me. However, it requires the classes to be added to the theme style. That would make the plugin almost useless for users that simply want to select a theme and some plugins without knowing HTML and CSS. Therefore I searched for a better option.

The second thought was to move the <style> tag before the <body> tag where it belongs. I cannot see any way how a sidebar plugin could do and I believe not sidebar plugin should ever do it. Especially not if it is a simply as the sidebarlogo plugin.

The third option was to search for ids or classes defined by default in any of them are approriate for sidebarlogo. This is true for image and description but not for all the other fields. However, using predefined classes as much as possible seems to be a good approach.

The last option was to use style attributes for each div generated by sidebarlogo. That would leave us with the same flexibility we have now plus producing valid HTML. However, someone pointed out that inline styles should be avoided whenever possible. Therefore, I introduced an option to either specify an ID a class or inline styles. This works nicely. The big downside for me was that now for each element available for layout there is another option for styling it. The long list of options that was confusing for me in the beginning now was much longer. And we had a lot of inline styles since no classes where available. Therefore I decided to drop some fields like sitename, copyright and contact since they can easily be generated within the description field.

From the blogs I know, an about box with some text and a image is pretty common. Some themes for s9y even provide such a box and this is how I started as well. But from my understanding this is not the way it should be. There should be a plugin for that. Sidebarlogo provides many other fields and can be confusing for someone you simply wants such an about box. Therefore I stripped its functionality. This is the version that is included in my pull request.

However, I do not like to break compatability. Maybe someone uses these fields. But yet, due to my changes and even before that, sidebarlogo is quite complex for the simple functionality it provides and especially regarding what many people want. Yet my answer to your question about HTML nugget is the answer why I am now working on a version of sidebarlogo that includes all these field and yet remains simply to use for someone who only needs basic functionality. This version will include all fields that were present in the original version as well as the option to sort them. Regarding this, the compatability will be reached with that version. However it still breaks compatability regarding the handling of style sheets. I cannot see how to solve that. What I can offer is a feature compatible plugin but not an option compatible one.

A last note on this new version: It will hide a the advanced option like the fields that are not commonly used, the style options and the sorting of elements in a group "More options" and "Styles". Therefore, the user will be presented with a really simple interface first and can switch to the full potential as needed. I consider this a very good approach since it neither cripples functionality nor overwhelms someone who simply wants to add an image with some text. Simple at first, more complex if needed. However, there is something I need to fix before I upload this version. Currently, the groups are displayed first before the ungrouped options. Is there a way to change that? This is the code I use:

Code: Select all

            $propbag->add('groups',        array('FRONTEND_FEATURES'));
            $propbag->add('config_groups', array(
                        PLUGIN_SIDEBARLOGO_GROUP_MOREOPTIONS => array(
                        'sitename',
                        'sitetag',
                        'contact',
                        'copyright',
                        'sequence'
                    ),
                        PLUGIN_SIDEBARLOGO_GROUP_STYLES => array(
                        'imagestyle',
                        'descriptionstyle',
                        'sitenamestyle',
                        'sitetagstyle',
                        'contactstyle',
                        'copyrightstyle',
                    )
            ));
I must admint that I do not understand the meaning of the first line nor could I find any explanation for it. I just kept it from sidebarlogo and added the groups the same way as I have seen it with other plugins.

Regarding your question concerning HTML nugget: Well, sidebarlogo itself is basically a little more than HTML nugget. It is simply more convenient for someone you does not know HMTL and CSS or does not want to mix content and layout. I like sidebarlogo because it allows me to select the image just by choosing it from my media library and changing text just by typing. No HTML there. However, this of course proves wrong my argument about stripping those extra fields. I said you could add them by using HTML but yes, then the persons is in the same position I do not want to be in. That it why I will reintroduce those fields as described above. Sidebarlogo in all versions is simply taking away the burden of writing HTML if all you need is a small about box. Yes, it has some more fields and options but that does not change the intention of the plugin. If you want to, I can upload some images to show the advantage of sidebarlogo over HTML nugget.

To sum up my rather long explanation:
My current proposal for sidebarlogo is neither option compatible nor feature compatible. After solving the question I have asked above there will be a new proposal that is feature compatible and adds a few features. However, it will not be option compatible regarding the styles sheets. As explained above, I do believe that this will not be possible. However, for someone who did change the CSS it will be possible to do the same with the new version. Users that did not change the default CSS will not even notice since the styles almost remain the same.

Regards,
Oliver

User avatar
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by Timbalu » Sat Nov 23, 2013 5:11 pm

Uff! :)
khor1701 wrote:Currently, the groups are displayed first before the ungrouped options. Is there a way to change that?
No, not really! That is how it is coded in functions_plugins_admin.inc.php.
The only way to cheat this, is to make all config options live in two or more config_groups. Then the array position decides which will be selected and showed first, I presume. Try that, though it would still require some additional javascript to have those general groups open unfolded.

I am not sure I did get all other questions. For the css you could add a

Code: Select all

            'css'                               => true,
to the $propbag->add('event_hooks', array( ... )

And a switch hook to function event_hook()

Code: Select all

                case 'css':
                    $tfile = serendipity_getTemplateFile('serendipity_event_sidebarlogo.css', 'serendipityPath');
                    if ($tfile && $tfile != 'serendipity_event_sidebarlogo.css') {
                        $eventData .= file_get_contents($tfile);
                    } else {
                        $eventData .= file_get_contents(dirname(__FILE__) . '/serendipity_event_sidebarlogo.css');
                    }
                    return true;
                    break;
Then you are able to put the styles into a serendipity_event_sidebarlogo.css inside the sidebarlogo plugin and can use an overwriting copy in your template.

Or without a file stylesheet like this

Code: Select all

                case 'css':
?>

.all your classes { }
/* or */
#ids {}

<?php
                return true;
                break;
khor1701 wrote:I must admint that I do not understand the meaning of the first line nor could I find any explanation for it.

Code: Select all

$propbag->add('groups',        array('FRONTEND_FEATURES'));
is used for grouping on spartacus: http://spartacus.s9y.org/index.php?mode ... EATURES_en
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian

khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Sun Nov 24, 2013 1:03 am

Hi Ian,
thanks for your reply and all your patience with me. Having the groups below the main options would be nice but is not necessary. I think I will simply stick with the common way. I just thought there might be an easy option. But if there is not, I do what everybody else is doing. It is best to match the standard.

The CSS stuff was not really a question. Sorry that you got me wrong here. Maybe I was unclear on that. I just tried to outline my thought on how and why I changed the plugin the way I did. The current approach is something I have seen on other plugins as well so it should be familiar to the user (except for the option to provide either an ID, a class or inline styles, but I explained it in the description and a default is set).

Okay, after all this is settled, I pushed to github and updated the PR. As I said before, the current version of sidebar plugin has the same features as the previous one plus some more. I grouped the options to make it more accesible for new users and changed the handling of styles to produce valid HTML. There are some minor feature added, but overall the functionality is the same as before. As I said, styles from the old version will not automatically work with the new version. Writing robust code for porting it to the new option is way to difficult. But as I mentioned before, I believe this to be a minor compatability issue.

Once again thanks for your help and I hope you find my changes benefitial.

Regards,
Oliver

User avatar
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by Timbalu » Sun Nov 24, 2013 3:23 pm

Ok thanks! Please assign the PR to @garvinhicking to let him decide the merge.
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian

khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Mon Nov 25, 2013 8:19 pm

Hi,
I am sorry, but I cannot figure out how to assign someone to a PR in github. Google isn't helpful on this either. Could you give me a hint or help me on that?

Regards,
Oliver

User avatar
Timbalu
Regular
Posts: 4598
Joined: Sun May 02, 2004 3:04 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by Timbalu » Mon Nov 25, 2013 9:06 pm

Just write a comment note to your PR and add @garvinhicking into it. This will assign your comment (and pull) request to somebody in special (meaning the one (Garvin) will be informed by email)
https://github.com/s9y/additional_plugins/pull/33
Regards,
Ian

Serendipity Styx Edition and additional_plugins @ https://ophian.github.io/ @ https://github.com/ophian

User avatar
onli
Regular
Posts: 2231
Joined: Tue Sep 09, 2008 10:04 pm
Contact:

Re: Update of Sidebar Logo Plugin or should I fork?

Post by onli » Mon Nov 25, 2013 10:26 pm

It is an admin option I think. I took care of that for you.

khor1701
Regular
Posts: 6
Joined: Fri Nov 15, 2013 1:45 pm

Re: Update of Sidebar Logo Plugin or should I fork?

Post by khor1701 » Sun Dec 01, 2013 12:00 pm

Just in case anyone stumbles upon this thread: The PR has been merged.

Post Reply