idea draft: template driven page list icons

[Soma]

Not sure you have seen it: http://wiki.processw...Module_Creation

Not sure if it covers everything yet, but we are still in the beginning of the advanced documentation (wish it were there when I began), so if you can find time and wanted to help it would be greatly appreciated.

Most things I learned by studying code and try out things, there's not really much about module once you get into it. People tend to make a BIG thing about it, when it isn't. After a couple modules you'll be much more comfortable.

Ahh PHP 5.3? I don't know if it's a good idea if the CMS is for php 5.2.4+.

Also since jquery core is not something I want to change, I slightly changed the .js you serve with the module to make it work.

I recoded your module to work, also with < 5.3 and replaced the hooks with alternative.

I recognized while playing with it, why you came to use the internal hooks to add script and css. The order of how scripts get added you can only control very little and depends when the module gets executed and what type of module it is, but I also got problems with js getting added when used in the module init(). I think it has to do with the autoload and not being a inputfield or process module. http://processwire.c...r-jquery-core/. Maybe time for Ryan to take a look at it again.

Adding script through a method in your module that will get executed when your hook method is being called is the way to go for this kind of module.

For the icon panel, you can use InputfieldMarkup to render markup. Also if you want to have the extra panel to be together with the select inputfield, you could create a InputfieldFieldset and put both inside. But in this case appending InputfieldMarkup to the form is ok.

I also moved the js init call into the .js file where it belongs to.

See attached the modified module code.

I really like this so far. Installed my 1'000 famfam icon set hehe. http://www.famfamfam.com/lab/icons/silk/

TemplateBadge.module.zip

[Soma]

Some random thoughts.

1. Maybe have the module configurable and be able to define a default icon or space for all in case none is set, to visually align the pages if only some have an icon set. Just thought it could make sense.

2. It would be handy to see the selected icon visually. Either the icons itself beside the select or highlighted on the icon table.

[mindplay.dk]

Well, thanks, but I intentionally build for PHP 5.3 ... my attitude is very simple: I build software for the future, not for the past. If I wasn't using newer PHP features, I wouldn't be using PHP at all - PHP has been playing catch-up the last couple of years, and has improved, but it's not exactly state of the art anymore. I don't think we further the use of PHP by showing the world what PHP code used to look like... You don't have to agree, but that's my philosophy

Yes, the module definitely needs configuration options, I thought about the default icon setting too.

As for the icon selection, I just did the quickest thing I could do - I might extend it later on, but for now, it gets the job done.

PS: forking on Github is always good!

[Nico Knoll]

Well I also used PHP 5.3 for my latest module. Because 5.3 adds "anonymous functions" which were important for it.

[Soma]

PS: forking on Github is always good!

I didn't mean to step on your module and philosophy. Just wanted to give an example.

However, you're right, so I created a fork and commited my changes for you to see easier. https://github.com/somatonic/TemplateBadges

[mindplay.dk]

I didn't mean to step on your module and philosophy.

I didn't at all mean to imply that you were - I just wanted to clarify where I'm coming from. Appreciate all your time and constructive input!

[mindplay.dk]

Couple of questions, trying to learn from your changes - thanks for posting the fork so I can see the diff.

About InputFieldMarkup for the icons - won't this cause the icons to go in a separate group from the dropdown control? That's what I was trying to avoid - that was the only reason they were mangled together like that.

As for the client-side, I see you were trying to use live() and bind() instead of on() - these are not at all the same as on() in newer versions, which performs much better, and provides a single, unified API call for all types of event management. I really hope we can have the latest version of jQuery in PW soon.

[ryan]

my attitude is very simple: I build software for the future, not for the past.

In a broader audience context, I think this would be more accurate to say "build software for the future, not the present". The reality is, we have a whole lot of people still on PHP 5.2.x, so that still represents the present to many in their environment. When building stuff for yourself, a client or smaller group, I think your attitude is the right way to go. But when building stuff for a worldwide audience, you only sacrifice your potential audience size by coding in a way that isn't backwards compatible with a still sizable chunk of active servers. Is it more beneficial to use an anonymous function, or is it more beneficial to be accessible to 20-30% more people?

For ProcessWire--a project where we are trying to grow our user base--accessibility with 5.2.x is a greater benefit than using 5.3.x specific syntax (though this will change quickly). I don't fault you for wanting to limit your module to 5.3 and above. But I think your statement is contextual to your situation, not ProcessWire's audience. Personally, I can't wait to be exclusive to 5.3 and above, but the time isn't right just yet. When every other site that someone asks me to check out isn't running on 5.2, then the time will be right, and it can't come soon enough.

I was less conscious of API choices and PW conventions - I tried to grok what I could from existing plug-ins, but as said, a code-review would be appreciated, if anyone has the time/experience...

I've gone through it and tested it all out and think it looks really good. Nice job! But since you asked for a code review, I'll make a few minor points:

1. There isn't consistency with use of starting "{" characters. Some start on their own line, others at the end of a line. ProcessWire's code standard is to never have a "{" character be the only thing on a line. But this is not applicable to 3rd party modules, so the only thing I would say is to pick one and stick with it.

2. There are very few comments in the module, and what comments there are most just point out the type for implied variable names. This isn't a problem for me, as I thought your code was strong and self documented very well without comments. But since we're discussing this in another thread, I thought I should point this out since you asked.

3. I have a little trouble following the code due to the narrow indentation, but I think that's only subjective and not something you need to worry about unless doing pull requests in the PW core or something. We use tabs rather than spaces in the core, but of course it doesn't matter what you choose to do in your own modules.

4. In your init() function, it would be preferable to skip attaching the hooks if they aren't going to be applicable in the request. What I would suggest is to add a ready() function and move all your 'addHook*' lines into the ready function. Then do something like this:

public function ready() {
 if($this->page->template != 'admin') return;
 $this->addHookAfter('...'); // and so on with the rest
}

Basically, there's no reason to attach the hooks you are attaching if the template isn't admin. You could even get more specific if you wanted to…

if($this->page->process == 'ProcessTemplate') {
 // add your ProcessTemplate hooks
} else if($this->page->process == 'ProcessPageList') {
 // add your PageList hooks
}

…but I think it's fine just to check the template. Ultimately, it's not a problem to stick with what you are already doing. But I'd consider it a best practice to apply some conditions to your hooks.

5. Other than being fun to do with PHP, I'll be honest and say I don't see the benefit of using a closure in your hookAddForm function vs making it a class function. I think the code would be more readable without it, but this is only subjective. So I'm just stating an opinion here, not suggesting you change it.

6. Your hookAfterRender function both adds a JS file and produces an inline script. Why not just move the contents of the inline script to your TemplateBadge.js file? This seems like unnecessary extra code that doesn't need to be in your .module file.

7. Moving into the UI: The select box could use a little more space between it and the icons (at least in Chrome):

8. You may want to add an 'author' property to the array returned by your getModuleInfo() function.

That's all I've got. I'm being picky here because I know that's what you want. But I also want to tell you what a great job I think you did with this module and how visually useful the result is. I love what this does for the PageList, so nice job!

I have updated the ProcessTemplate::buildEditForm to be hookable and it should appear in the source later today. I'm testing out the latest jQuery in ProcessWire, but running into some problems there. For instance, why does a statement like this stop working in jQuery 1.8?

var $buttons = $("#content a[id=] button[id=], #content button.head_button_clone[id!=]");

jQuery 1.8 returns a blank array [], but jQuery 1.6 and prior return the items that match the selector. It's these little changes in jQuery that drive me nuts, and the reason I don't upgrade immediately every time they release something new. But unless I'm missing something (very possible), this seems like a completely valid selector that breaks under jQuery 1.8.

My best guess is that jQuery 1.8 now treats "id=" as "has an ID attribute that is blank", whereas previous versions treated it as "has an ID attribute that is blank OR has no ID attribute", but haven't yet confirmed.

[Soma]

Couple of questions, trying to learn from your changes - thanks for posting the fork so I can see the diff.

About InputFieldMarkup for the icons - won't this cause the icons to go in a separate group from the dropdown control? That's what I was trying to avoid - that was the only reason they were mangled together like that.

What if you try it out and see?

I see why you did that. The way with InputfiledMarkup it creates a new container below the dropdown simply. There's no real problem I see with having them separated. But following the Inputfield strategy, you could add a InputfieldFieldset (as mentioned already ealier) and add the select and the icon markup to they group together. The way you did it is also possible obviously just wasn't working since I don't run 5.3 locally yet. And I think it's easier to follow using inputfields.

As for the client-side, I see you were trying to use live() and bind() instead of on() - these are not at all the same as on() in newer versions, which performs much better, and provides a single, unified API call for all types of event management. I really hope we can have the latest version of jQuery in PW soon.

I know. Well I wasn't trying, I just quickly replaced them with the old event methods so it works without updating the jQuery core (which would break PW admin). I agree it would be cool to use latest event methods, but this isn't yet possible so.

[diogo]

but I think that's only subjective and not something you need to worry about unless doing pull requests in the PW core or something

I don't think I will be contributing for the PW core in the near future. But would be nice to have some documentation on coding standards for those who will. Something like this: http://dev-docs.fuelphp.com/general/coding_standards.html

[Soma]

The .css doesn't get added on the link dialog in TinyMCE where you can browse the page tree to select a page to link to. So the icon and badges have no styling.

I think just adding the .css always in the admin would be ok.

[mindplay.dk]

I'm not sure what you mean, but I would accept a pull request

[Soma]

Nothing fancy, just load the .css always in the init() solves the problem.

What are your plans on this module making it work without changing core jquery and php 5.3? Will you bring in the changes needed or should I also request a pull?

I was also thinking how custom themes that already uses icons, that we could handle it so that with this module they will get disabled. I think overwriting css would be a way.

[mindplay.dk]

I have no immediate plans to downgrade code to an older version of jQuery - I'm in no hurry, so I'll wait for PW to receive an upgrade. I'd rather not downgrade code now, and then have to upgrade it again when a newer version of jQuery is integrated.

I think your theme is one of the only ones that have icons? That was one of the first reasons I liked it ... it's probably a quick fix for someone who doesn't want them, to remove the icons from the theme? - ideally, I guess the theme should provide an option? (or you could ship the current icon functionality, with the theme, as an optional plug-in)

[mindplay.dk]

Looks like ryan is upgrading jQuery

[mindplay.dk]

TemplateBadges is now TemplateDecorator.

I've pushed up a new version with some new features - it now has a configuration screen where you can configure a default icon. I've included a "blank" and a simple "page" icon which are good choices for default icons. There is also an option to display the template name when editing a page.

The tab is now called "Decorations", and has an added field for directions - if you add directions, they will display near the top of the "Content" tab, when editing a Page with that Template.

The Github page is open to submission of issues - post'em if you got'em.

Note that this was tested with the current dev-branch of PW, and works without modifications.

[ryan]

Thanks for the updates! I think you've got a good name there, but wanted to mention another possible alternative: PageListDecorator ? I only mention it because what it's doing is decorating the items in the Page List, so when you mentioned the name change it popped into my head. Though I think either name works. When ready, you might like to add this one to the directory at http://modules.processwire.com/add/

[mindplay.dk]

It also decorates the page editor - and all the decorations are template-specific. But yeah, either name would work

I think I should wait and add it to the directory after the next PW release, since it works only with the dev-branch at the moment - it would raise too many questions if released now.

[mindplay.dk]

Pushed up another update, integrating some of your suggestions - cleaned up some redundant hooks, made the CSS file global so the Page List Inputfield displays correctly, added lots of documentation.

I decided to add @see annotations to every hooked method - this documents for every method which method in ProcessWire it's hooking. Seems really logical - enables those with an IDE to click through directly to the hooked method. Perhaps this should be considered "mandatory" practice when writing modules?

[mindplay.dk]

@Soma what do you think about adding custom icons for home, trash, hidden, unpublished, etc. to this module? This would complete the remaining icon logic from your admin-theme. For the default icons, I like the ones you're using because they're gray - so I could pull those in and make them the standard icons if you'd like.

You could then retire the icon-functionality from the theme? Seems almost like functionality/logic - it's bordering on a "feature", perhaps more so than just look and feel. Perhaps this kind of thing is better left to modules than to themes?

I'm asking your (and others) opinion here, not trying to dictate anything

[ryan]

I think I should wait and add it to the directory after the next PW release, since it works only with the dev-branch at the moment - it would raise too many questions if released now.

We're probably only a few days from bringing in the new jQuery into the master branch. So far I've only come across the one issue I mentioned before and nothing else has turned up yet. If that's the only issue, I'd consider that fairly minor (easy to fix). So no reason to delay further on bringing in newer versions of jQuery and jQuery UI.

[Martijn Geerts]

I Think icon fonts is the way to go for pageList icons. And then use them as pseudo elements. This way You're free of screen density & color etc.

Then if used a module, everyone could blend it in within their theme.

[mindplay.dk]

I Think icon fonts is the way to go for pageList icons. And then use them as pseudo elements. This way You're free of screen density & color etc.

In an ideal world, I would agree - however, this has a number of drawbacks: Icon fonts are not widely available, and do not support colors. From my understanding, there's also a dependency on fairly new browsers. There may be licensing issues. But perhaps most importantly, creating or editing fonts is not something that most people know how to do - there are few, highly specialized designers that have the tools and talent to create fonts correctly this way. Bitmap icons, on the other hand, are widely available, and there are plenty of (free) tools to create and edit them, and it does not require hard-earned skills.

By the way, bitmaps do scale down nicely in most browsers - monochromatic PNG compress extremely well, so you could accomplish something similar in terms of scalability, using higher-resolution PNGs. If I cared enough to find out, this would be the approach I would pursue

[Soma]

You could use @x2 versions and with some css. I also thought at first it would be cool to use icon font. But for various reasons also mindplay mentioned it's really a lot more easier to use bitmaps. The way it is done is nice and allows to easily change and add icons. Perfect for this usecase in the page list tree. Icon font would be nice for admin interface icons and for frontend.

@mindplay I'm not sure how to handle the theme conflict with the module. Overwriting via CSS would be an option, since my and other themes use CSS to add icons to the div wrapper as background. You module add an img to each page via the label. This is a nice thing.

That's what I meant that I would have done it differently and basicly use CSS styling for adding icons same as with the theme and overwrite them so to say. This would have also worked, but the current solution is also nice I think. Also, as with this thread, Ryan added a css class to each page in the tree with it's template name especially for being able to do something using css only.

Since the admin theme is not a module and I don't want to add a module together with the theme the user also needs to install. No option for me.

1. So either remove my icons from the theme completly, and be able to add icons with this module.

2. Or add a line or two in my theme to check if your module is installed and then not ouptut the css for the theme icons.

3. Your module overwrites css for the pagelist tree to remove icons.

As for the states. For the hidden pages it already has opacity changed as by default in PW, and unpublished are strikethrough. So this is already covered, and for having separate icons for each state I think it would be overkill.

[mindplay.dk]

Thanks for your input, Soma.

For now, I decided to simply fork your theme and get rid of the icons - I did two commits on a fork here, one that removes the icons, and another that renders the lock/unlock icons in black. (note that there's also a "key" icon in PW PageList module, which does not appear to be in use??)

I don't want my module to contain workarounds for any specific themes - if other themes add icons or something else, going down that path leads to increasing maintenance effort.

I agree, separate icons for states that are already reflected by typography or opacity, not necessary. It would be nice to have icon choices for the "system" pages though - e.g. admin, trash, etc... although I suppose you could already turn on display of "system" templates and simply add the icons to them? So perhaps this is not needed at all. Might be nice if the installer configured those for you on install though - I looked into that, but I'm not sure how to configure a module at install... (is that possible?)