This issue is basically a placeholder for what I hope will be the last large blocking issue before the 3.x alpha is released. Much of the theme code for 3.x is "disconnected" - the code is there, but it's not used anymore because we use views instead. We need to remove the disconnected code if it's no longer needed, or port it to the new 3.x release.

Admittedly, theming is my weakest point in Drupal, but I learn best when I have an itch to scratch. I've got some reading to do :)

CommentFileSizeAuthor
#10 ng3-css.patch1.88 KBjustintime
#5 ng3-image-navigator-template.patch8.03 KBjustintime
#1 ng3-image-navigator-template.patch7.66 KBjustintime

Comments

justintime’s picture

justintime commented
Assigned: Unassigned » justintime
Status: Active » Needs review
StatusFileSize
new ng3-image-navigator-template.patch7.66 KB

All unused code has been removed.

Here's my first crack at moving the navigator into a PHPTemplate file. Never before have I spent so much time working on something that looks the same now as it did before I started :)

I did change a few very minor things. I'm attaching the image nids of the gallery to the navigator array for use in the theming layer - it's cached data so it's "cheap". I changed the naming of a couple classes to make more sense. The "Back to gallery" link was actually an unformatted list, and was adding a line break to the navigator. I fixed that, and the result is that the navigator "bar" is half the height it used to be. I think it looks much better. I also send in the gallery title to help with #725836: Modify "Back to gallery" link.

I really need some review on this patch, as it's my first adventure in theming. Pat me on the back or slap me in the face - I can take it :) Instead of setting the status to RTBC, please set back to active if approved, as I need to do a lot more work before this issue is done.

justintime’s picture

justintime commented
Title: Remove unneeded theme code, refactor 3.x theme code » Refactor 3.x theme code

Updating title to reflect work not yet committed to CVS.

scroogie’s picture

scroogie commented
Status: Needs review » Active

This looks very good. I really prefer using template files.
Some remarks:
- great work! This also clarifies the code a lot (the old theme function was ugly).

- you are removing the image case from hook_operations. The whole function is for VBO, or not? Can't we please drop the whole function and the rest of the VBO stuff? I mean, this is what manage images is for, or not? I'd rather have the hierarchy stuff in NG core, than the VBO stuff. :D

- does this need another update function where we clean the caches?

- is 'parent' and 'parent gallery' the right name for the CSS class and variable? You use the class name 'parent-gallery' but the variable 'gallery_link' and the $navigator key 'parent'. Perhaps we could make this consistently use 'gallery_link' or 'gallery-link', and the key in the navigator array 'gid' or even better 'gallery_nid'?

- I might be mistaken, but wasn't there a link href somewhere, that brought the browser to the height of the image navigator when clicking? I don't remember exactly.

- does the Views field still work? We pass $node to the theme function in node_gallery_views_handler_image_navigator.inc. Perhaps this is not required anymore? (the template preprocess function only takes the variables array). Or did you just want to make this available to the template (which sounds good). We just need to add it to the variables list then.

Cheers
scroogie

justintime’s picture

justintime commented

Thanks, I can certainly see the convenience of template files after working with them. Here's responses to your points above.

- node_gallery_operations() isn't a hook, it's just a function. It's not referenced by any code any longer, save for one view handler. I removed the case from the function because it wasn't being used anymore. I'd love to get rid of the function altogether. Re: VBO - there's actually very little VBO-specific code in the module -- VBO relies completely on Drupal Actions. Actions are a good thing to keep around, because VBO can use them, so can rules, triggers, and other modules.

- I don't follow what 'this' is when you say "does this need another update function where we clean the caches?"

- I agree we should make our indices consistent, stay tuned for an updated patch

- Re: anchor tags in the link, those are still there but handled in the preprocessor function. See theme.inc lines 40-44.

- I completely forgot about the views field. I'll fix that as well.

justintime’s picture

justintime commented
StatusFileSize
new ng3-image-navigator-template.patch8.03 KB

The views field still works - the node gets passed in as $variables['image'] - it's just the way the theme system works. Here's a fixed up patch implementing the changes discussed above.

scroogie’s picture

scroogie commented

Re: _operations()
Indeed, I confused that. Our function is actually called only with 'cover' as $type, so I wonder what the rest is there for. Neither the function, nor the views handler seems to be called in any situation (I put some debug code there). So I just assumed that it was for VBO. That was apparently a wrong assumption.
About actions: It adds 9 or so functions to the module. I can see the value of actions, but whenever would you set an action that changes an imageweight to a constant value? Or that changes the gallery the image is in? The image weight also has some ugly code, look at node_gallery_change_image_weight_action_form(). This is because it actually mis-uses actions here. Afaik, actions should always act on one node only. To act on a set of nodes, Drupal uses the "Operations": http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo...
Perhaps we can clean up the VBO stuff from the actions and put them in a seperate file node_gallery.actions.inc or something?


- I don't follow what 'this' is when you say "does this need another update function where we clean the caches?"

I just meant the whole patch, because theme stuff is cached.

justintime’s picture

justintime commented

I committed a removal of node_gallery_operations().

Re: clearing cache, once I get all the theming stuff done, I'll write the update hook to clear caches.

Re: actions - let's stay on topic. I started #973532: Actions and VBO discussion to discuss actions and vbo.

scroogie’s picture

scroogie commented

The changes look good btw. Sorry for sidetracking again (I need to be more disciplined with this). :)

justintime’s picture

justintime commented

I've committed the navigator updates, and I took a crack at theming the manage images form/page. I had a heck of a time getting anything to really work - either I did so much in the preprocess function that there wasn't much left to override in the template, or it looked worse, or it had bugs. So, I left a kind note for the first theme dev who figures out how to do it properly :)

justintime’s picture

justintime commented
Status: Active » Needs review
StatusFileSize
new ng3-css.patch1.88 KB

Here's a touch of CSS to spruce things up a bit. Thoughts?

scroogie’s picture

scroogie commented

Looks great!
Can we also have a 

div.node-node_gallery_image div.field-field-node-gallery-image {
  text-align:center;
}

to center the image on node display, or is this a no no?

justintime’s picture

justintime commented

Nope, that should be fine, I just hadn't got to the node theming yet. Also, note-to-self: need to apply CSS to the blocks provided by views.

justintime’s picture

justintime commented

I've just committed the css tweaks. Anything left to do here? There is #972678: Consolidate Views? that may impact template files, but I don't think we should keep this issue open for that.

justintime’s picture

justintime commented
Status: Needs review » Fixed

Marking fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.