Posted by Eric_A on April 8, 2010 at 1:02pm
3 followers
| Project: | Chessboard Renderer |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Eric_A |
| Status: | closed (fixed) |
Issue Summary
Currently themes cannot override anything. There is no theming layer in Chessboard Renderer.
The function chessboard_render($content) implements the parsing of the original value and the rendering to html. I propose to design a preprocess function and a theme function and then refactor chessboard_render() to use the theme layer.
A simple patch would reuse as much variables as possible. The one variable I think we should add is a two-dimensional array with pieces instead of having themes parse the chessboard string notation syntax.
In case of an empty board the template would receive something like this:
$table_xhtml = array(...);
$file_max = 8;
$square_color_first = 0;
$border = array(
'T' => TRUE,
'B' => TRUE,
'L' => TRUE,
'R' => TRUE
);
$piece_placement = array();
Comments
#1
#2
This patch for the head of the trunk shows the idea. I'll submit one for DRUPAL-5 later.
It basically wraps existing code in a theme function. I decided to leave chessboard_render() alone for 2 reasons:
-keep this first step as simple as possible.
-do not introduce coupling between the (semi) external library file and Drupal theming mechanisms.
#3
Oops. The above patch was the wrong one.
#4
This one takes care of DRUPAL-5 and fixes the instance of #chessboard_notation.
#5
#5 is diff -up whereas #4 was diff -u. So no more excuses for not reviewing, LOL.
Removing stuff that is out of scope:
-changed theme function name from chessboard_notation to chessboard, showing better that theme('chessboard') is basically the same thing as chessboard_render();
-replaced semi-element structure with a simple array. (Signature still upward compatible with D7.)
This patch not only brings us theming, it also removes some code duplication.
#6
Part of the patch is about moving code around. For the sake of simplicity I have not changed the coding style to the latest guidelines. (http://drupal.org/coding-standards) Maybe the best approach is to do that in one go for the whole project in a seperate issue.
#7
Adding a theme layer means that chessboard.css now really should not be added by
hook_menu()(orhook_init().)#8
In this patch the formatter is responsible for both picking a theme and handling the css. Also, the filter system no longer duplicates formatter code, but instead there is a direct call to the implementation of hook_field_formatter(). Hmm, perhaps another pair of eyes?
#9
Fixes the concern about directly calling a formatter by not calling it at all and just duplicate those two lines of code...
A call to content_format() is another idea for another time.
#10
Committed to CVS DRUPAL-5.
#11
Maybe the loading of the CSS should be done in the theming function itself. If someone overloads the theme, probably they will not have any use for the CSS file anyway.
Or is it common practice in Drupal to do it like in the patch?
#12
Never mind. Apparently you fixed it in HEAD already (calling
drupal_add_cssinsidetheme_chessboard).#13
Maybe the loading of the CSS should be done in the theming function itself. If someone overloads the theme, probably they will not have any use for the CSS file anyway.
Yes, I agree regarding this css file.
Or is it common practice in Drupal to do it like in the patch?
Well, css files are added all over the place. By hook_init()/ hook_menu(), by theme functions and templates, by preprocessors and by render elements (D7 #attached property). For Chessboard D7 this theme function specific css will probably be moved into a preprocess function.
#14
Actually, .chessboard .row is more like element type level css. In that case it should be added like in the patch... We need a separate issue for this.
#15
Currently CSS is broken, because the filter works in cache mode and the theme function thus is called only once. (The css is there only the very first time you view the board.)
Changing our filter to a 'no cache' filter allows the theme function to add the CSS but disables caching for the entire format...
#16
Automatically closed -- issue fixed for 2 weeks with no activity.
#17
Some further refactoring has been done. Most notably the legacy chessboard_render() has been deprecated and that code has been split up. We now have an item that gets expanded/sanitized, formatting code, a template preprocessor and template. See also bug #878164: Function chessboard_render() should not add stylesheet.