CVS edit link for tnlone

I have developed a simple blank canvas so I should call it a starter theme which is very simple in nature yet has some advanced theme options which may or may not be present in another themes. I had novice theme developers in mind when I designed this theme. Simple reason was that when I started using Drupal, I had very little idea about any coding language or any theme development, over time I learned from the Drupal API documents and community contribution.
However, the hardest part was how to set a simple layout without working out negative margins or complex layout structures. I always wondered if there was a possibility I could change my layout as many times as I wanted without going through the CSS files.
So I decided to contribute to Drupal community a starter theme which will be very simple in nature yet will have powerful options.
I have finished working on the beta version of the theme system, and it's production ready 100%. However saying that I would not say that it may not come across an issue in future, which I am willing to cater to.
The name of the theme system is Drustart, the motivation behind the name was Drupal and Starter theme.
The theme system's layout is defined by theme settings and some of the CSS properties are also defined from theme settings page, thus giving the end user the ability to just change the theme setting to start the theme building.
It also has some theme snippets usually requested or looked for by community.
My motivation is simply to give novice users the ability to design their own theme to harness the power of Drupal system, without being tied down to scrolling through 100's and 1000's of lines of CSS properties and template files. A user should be able to change some basic layout setting just from the theme settings. Hence the birth of Drustart.
I hope I get the opportunity to contribute my work to the plumbing community.
CommentFileSizeAuthor
#26 drustart-6.x.tar_.gz185.15 KBtnlone
#22 drustart-6.x.tar_.gz185.37 KBtnlone
#18 drustart.tar_.gz176.83 KBtnlone
#11 drustart-6.x-1.1-alpha1.tar_.gz177.63 KBtnlone
#8 drustart.tar_.gz158.21 KBtnlone
#6 screenshot-large.png95.82 KBtnlone
#6 screenshot.png14.86 KBtnlone
#6 drustart.tar_.gz153.64 KBtnlone
#1 drustart.tar_.gz65.06 KBtnlone
#1 screenshot.png15.29 KBtnlone
#1 screenshot_large.png74.71 KBtnlone

Comments

tnlone’s picture

tnlone commented
StatusFileSize
new screenshot_large.png74.71 KB
new screenshot.png15.29 KB
new drustart.tar_.gz65.06 KB

Please find attached the finished work for Drustart theme system with screenshots

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Postponed (maintainer needs more info) » Needs review
Issue tags: +Theme review

Don't forget to change the status, when you upload new code.

tnlone’s picture

tnlone commented

Thanks for the reply,

tnlone’s picture

tnlone commented
Assigned: Unassigned » tnlone
Status: Needs review » Active

it's status is that it needs more info, do you guys need more information regarding the theme system...or more on motivational message..just to be on a safe side I am adding information on the theme system:
---------------------------------------------------------------------------------------------------------------------------------------------
Introduction to Drustart

The Drustart theme came from an idea that theme development should be easy and creative for people with very little skills. The purpose here is to have a very minimal CSS based theme that contains functions that are used for most advanced websites. This theme is not intended to have
subthemes, or to be another version of any parent theme. Drustart is only intended to provide an extremely clean and flexible start for a Drupal themer without consuming lot of time and effort.

__________________________________________________________________________________________

Drustart Feature List (targeted for 6.x only at this stage)

- flexible and simple info file

- Body classes.

1. front or not-front classes
2. logged-in or not-logged-in classes
3. node-type-CONTENT_TYPE class: for example, node-type-page, node-type-story and node-type-forum
4. two-sidebars, one-sidebar sidebar-left, one-sidebar sidebar-right, or no-sidebars classes
5. page-URL class
6. section-FIRST-Arg class
7. section-node-add, section-node-edit, or section-node-delete classes for node add, edit, and delete pages

- Node classes.

1. node-mine
2. node-type
3. node-unpublished
4. ntype-[node type]
5. taxonomy

- Block Classes

1. block-[block module]
2. region-[region]
3. odd / even

- Comment classes

1. node-author
2. odd / even
3. new
4. mine

- Block editing links. Users with permission to edit blocks will see, when
hovering over any block, links to edit that block. This is much more intuitive
than first going to admin/build/blocks.

- Minimal regions : Header / Footer / content / sidebar left / sidebar right
- 3/2/1 columns layout
- liquid or fixed layout by just changing options in the theme settings
- No negative markups or margins for sidebar placing

- clean folder architecture : css / images / templates /functions /
- themable breadcrumb
- themable pager
- custom feed icon
- themable comment links
- Advanced seach options
- themeable button classes
- Accessibility features
- CSS file inbuilt, reading to edit

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Needs review

The status is needs review, which means somebody needs to review the code. The status active is never used on this issue queue.

tnlone’s picture

tnlone commented
StatusFileSize
new drustart.tar_.gz153.64 KB
new screenshot.png14.86 KB
new screenshot-large.png95.82 KB

Sorry kiamlaluno,

My bad, however, thanks for the info, I will keep that in my mind for future reference...I am attaching a new gzip file to this reply, with new screenshots. I have changed some options for better UI and fixed some issues of conflict with some modules.
I also cleaned up a lot of code in tpl.php files and CSS files.

Thanks..

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: tnlone » Unassigned
tnlone’s picture

tnlone commented
Assigned: Unassigned » tnlone
StatusFileSize
new drustart.tar_.gz158.21 KB

Sorry to be a pain kiamlaluno,

However the last upload had an error in it's template.php file on line 610 and 613. I had forgot to comment in body classes sidebar-last and sidebar-first. Which was casing layout error.

Well now I have tested this in many ways, from a fresh install of whole drupal site to adding modules. Also got it validated, it passed valid XHTML 1.1 and XHTML 1.0 Strict!. as well as CSS level 2.1.

Well fingers crossed I hope no errors are generated.

PS I tried to add status to the previous reply however it didn't add up... :-(

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: tnlone » Unassigned
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work

You are defining phptemplate_preprocess_node(), and drustart_preprocess_node(); I am not sure what would happen in such cases. Why isn't the code simply defining of the preprocess functions.

The file LICENSE.txt needs to be removed; Drupal.org CVS doesn't allow to commit that file.

tnlone’s picture

tnlone commented
Status: Needs work » Fixed
StatusFileSize
new drustart-6.x-1.1-alpha1.tar_.gz177.63 KB

Hello kiamlaluno,

Hope you had a good Christmas and a new year.

I have had some personal problems and work has been very hectic over past few weeks, hence the reason for such a delay in response to your query. The reason I had two preprocess functions defined was simply a negligence error, last time on upload of the code I clearly have missed it. *Would keep this in mind for future reference *
I have cleaned up a lot of the architecture of the theme system. I have made some changes to the template files. I have also removed the license.txt
I have for my reference named it alpha 1
Please find the new code attached.

Regards

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Fixed » Needs review
tnlone’s picture

tnlone commented

Hello Kiamlaluno

I was just wondering what's happening, as there has been no activity on this issue for past three weeks and the issue is now nearly two months old...I was wondering if there is anything that needs to be done or anything...

Thanks in advance

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
  1. function drustart_settings($saved_settings) {

  // Only open one of the general or node setting fieldsets at a time
  $js = <<<SCRIPT
    $(document).ready(function(){
      $("fieldset.general_settings > legend > a").click(function(){
      	if(!$("fieldset.node_settings").hasClass("collapsed")) {
          Drupal.toggleFieldset($("fieldset.node_settings"));
      	}
      });
      $("fieldset.node_settings > legend > a").click(function(){
      	if (!$("fieldset.general_settings").hasClass("collapsed")) {
          Drupal.toggleFieldset($("fieldset.general_settings"));
      	}
      });
    });
SCRIPT;
  drupal_add_js($js, 'inline');

    The JavaScript code should be place in a separate file. 

  2.   $defaults = array(
    'user_notverified_display'              => 1,
    'breadcrumb_display'                    => 'yes',
    'breadcrumb_separator'                  => ' &#187; ',
    'breadcrumb_home'                       => 0,
    'breadcrumb_trailing'                   => 0,
    'breadcrumb_title'                      => 0,
    'search_snippet'                        => 1,
    'search_info_type'                      => 1,
    'search_info_user'                      => 1,
    'search_info_date'                      => 1,
    'search_info_comment'                   => 1,
    'search_info_upload'                    => 1,
    'taxonomy_display_default'              => 'only',

    The settings should have the name prefixed by the theme name, to avoid conflicts with settings injected by third-party modules. 

  3. t('!name', array('!name' => t($name))),

    The first argument of t() must be a literal string, or the script that extract the strings to translate will not extract any string to translate (and it would be like t() is not even invoked). Also, the code should simply be t($name), but as I reported, that doesn't work. 

  4.   // Read more link settings
  $form['drustart']['node_type_specific']['link_settings']['readmore'] = array(
    '#type'        => 'fieldset',
    '#title'       => t('"Read more"'),
    '#collapsible' => TRUE,
    '#collapsed'   => TRUE,
   );

    Avoid to align the code like that; each space you add to align the code makes the memory required higher, without any good reason. 

  5.    $defaults = array(
    'user_notverified_display'              => 1,
    'breadcrumb_display'                    => 'yes',
    'breadcrumb_separator'                  => ' &#187; ',
    'breadcrumb_home'                       => 0,
    'breadcrumb_trailing'                   => 0,
    'breadcrumb_title'                      => 0,
    'search_snippet'                        => 1,
    'search_info_type'                      => 1,
    'search_info_user'                      => 1,
    'search_info_date'                      => 1,
    'search_info_comment'                   => 1,
    'search_info_upload'                    => 1,
    'taxonomy_display_default'              => 'only',
    'taxonomy_format_default'               => 'vocab',
    'taxonomy_enable_content_type'          => 0,
    'submitted_by_author_default'           => 1,
    'submitted_by_date_default'             => 1,
    'submitted_by_enable_content_type'      => 0,
    'readmore_default'                      => t('Read more'),
    'readmore_title_default'                => t('Read the rest of this posting.'),
    'readmore_prefix_default'               => '',
    'readmore_suffix_default'               => '',
    'readmore_enable_content_type'          => 0,
    'comment_singular_default'              => t('1 comment'),
    'comment_plural_default'                => t('@count comments'),
    'comment_title_default'                 => t('Jump to the first comment of this posting.'),
    'comment_prefix_default'                => '',

    That code is present in two different files. Isn't possible to keep it just in a single file that is included where required?

tnlone’s picture

tnlone commented

Thanks for your review, however, I have some queries in regards to your review:

  • 01. The JavaScript code should be place in a separate file
    Here you are asking me to make a separate file for a small inline js, which according to my understanding of coding is not in anyway malicious or in-practicable in nature, lot of themes and modules already committed to Drupal CVS have some js inline. Then you are asking That code is present in two different files. Isn't possible to keep it just in a single file that is included where required? here now you are asking me to merge the variable into a single file. It to me does not make any sense. If we have a rule of something I would presume it would apply to other.
  • 02. Avoid to align the code like that; each space you add to align the code makes the memory required higher, without any good reason I do understand the implication of having spaces in files related to higher memory consumption, however then you are asking me to create bigger variables by The settings should have the name prefixed by the theme name, to avoid conflicts with settings injected by third-party modules. I also understand this concern, but then doesn't having long variable slow down the page load and increase memory consumption. I only aligned code in this format because it's a starter theme and from the motivation point of view I wanted a clean architecture because this theme was not targeted for advanced user like yourself. But the novice users of Drupal who want to play with the themes and learn what's what.
  • 03.The first argument of t() must be a literal string, or the script that extract the strings to translate will not extract any string to translate (and it would be like t() is not even invoked). Also, the code should simply be t($name), but as I reported, that doesn't work.
    Here I am not sure what exactly are you referring to are you asking me to change the t() or you are just letting me know it doesn't work from your point of view or Drupal's point of view. Also which area are you referring to in my theme.

    • I am not whining about your criticism of my work, you are doing what you have to do, however, after waiting for more than 2 months I would have thought there would have been major coding errors in my theme that it too so long for a response, these are very small issues or I should say feature requests. I do understand that you guys are volunteers but then so are we who contribute to the Drupal.

    However thanks for review of my work, I will work on redevelopment of the theme and upload the code very soon...

    avpaderno’s picture

    avpaderno
    he/him
    Primary language Italian
    Location Brescia, 🇮🇹 🇪🇺
    commented
    • To avoid conflicts with existing modules is what it's called namespace respect, which is something required from coding standards. Each module needs to live together other modules, and avoid any conflict, which means also to avoid to use Drupal variables with a name that is not start with the module name.
    • If you use code like $output = t($string), you get the same effect than writing $output = $string; in other words, the string is not translated, if not when another module is using the same string (which could happen, or not). Plus, The translation server (used on localize.drupal.org) would report an error, suggesting to change the code.
    tnlone’s picture

    tnlone commented

    Thanks for your reply...I am working on the revamp of the theme, I should submit it as soon as possible for your kind perusal.

    tnlone’s picture

    tnlone commented
    Status: Needs work » Fixed
    StatusFileSize
    new drustart.tar_.gz176.83 KB

    Please find attached the new code, as per your previous comment I have done the following changes :

  • Changed the settings with theme name prefix, also just to mention even prior changing it, the settings had an auto option to check for other modules and if it did find any conflicting one, it would have disabled the feature in the theme settings form giving the user message to why the feature is disabled.. Which to my understanding would have been sufficient enough. However as the old age saying goes you can never be over prepared.
  • Removed the inline JS from the theme-settings.php
  • Merged the theme setting defaults into one file
  • Removed a lot of white spaces so not should not be an issue of code alignment
  • As far as your concern for the t('!name', array('!name' => t($name))), I checked it with the translation it has not bearing on the translation, as the variable did get translated in to different languages I installed on my testing environment. So I have left it unchanged, as that means total rewrite of my code. Also that statement is being used by many other themes already committed to the Drupal CVS.

    • Well I hope these are the final changed I may need to do, and I may get an answer a bit sooner...as my application is now nearly three months old.

    Regards
    TNLone

    tnlone’s picture

    tnlone commented
    Status: Fixed » Needs review
    avpaderno’s picture

    avpaderno
    he/him
    Primary language Italian
    Location Brescia, 🇮🇹 🇪🇺
    commented
    Status: Needs review » Needs work

    I checked it with the translation it has not bearing on the translation, as the variable did get translated in to different languages I installed on my testing environment. So I have left it unchanged, as that means total rewrite of my code.

    It gets translated just because the used string is also used by another module, not because the script that creates the translation templates would find that string in your code; if no modules would use the same string, then that string would not be translated.
    Then, t('!name', array('!name' => t($name))) has the same effect of t($name), and (when no module uses the same exact string) $name. t($name) would be reported as error from the localization server.

    That needs to be changed in the code.

    tnlone’s picture

    tnlone commented

    If this needs to be changed in my code...so shall all other Drupal Themes committed to CVS be asked to change' as this code is present in another projects. I think personally all you are trying to is deny my application or make it harder, if such is the case, reject the application which you as a reviewer can do... I do not care, I can contribute to community with out having a CVS account.
    This is where open source fails... Clearly as I have seen from your reviews to other issue queues, it seems you are very indifferent towards my application. My application has been 3 months old and you have no concern or regard or maturity for the patience I have shown till now. While many other application where approved by you in a week or so, with apologies from you in regards to time taken. Also you have even approved applications where the code had not been fixed yet. I do not question your motivation in doing such as I do not want to dwell into that water.

    I personally don't think I have violated any coding principles as laid by Drupal :- http://drupal.org/coding-standards.

    Classic example is the base theme Garland shipped with Drupal code itself..

    function phptemplate_comment_submitted($comment) {
  return t('!datetime — !username',
    array(
      '!username' => theme('username', $comment),
      '!datetime' => format_date($comment->timestamp)
    ));
}
function phptemplate_node_submitted($node) {
  return t('!datetime — !username',
    array(
      '!username' => theme('username', $node),
      '!datetime' => format_date($node->created),
    ));
}

    where the string $output = t($string) is being used...

    So I am not going to change it till I see other codes changed, it's matter of principle. If it applies to me, it should apply to everyone, as it's very hypocritical of you to ask one maintainer to change all his code and have a blind eye to other maintainers. If this is not an issue for Garland theme why is it an issue for my work because I do not understand where is it different from the string used by me
    '#title' => t('!name', array('!name' => t($name))),

    Also if this really is an issue why was it not pointed out in http://drupal.org/node/654288#comment-2440128 earlier, I may have worked on it then. But now after silence of a month you have issues with it...

    tnlone’s picture

    tnlone commented
    Status: Needs work » Needs review
    StatusFileSize
    new drustart-6.x.tar_.gz185.37 KB

    I have changed code from 

      '#title' => t('!name', array('!name' => t($name))),

    to
    '#title' => t('!name', array('!name' => $name)), that's all I can do...After this alteration is beyond the scope..

    Anonymous’s picture

    Anonymous (not verified) commented
    Status: Needs review » Needs work

    First and foremost reviewers will be looking for a good understanding of both security related issues and Drupal's API.

    CVS applications review, what to expect

    If you write code like '#title' => t('!name', array('!name' => t($name))), I get you didn't correctly understand how the function t() is used, and when.

    As you make a comparison between

      return t('!datetime — !username',
    array(
      '!username' => theme('username', $comment),
      '!datetime' => format_date($comment->timestamp)
    ));

    and 

      '#title' => t('!name', array('!name' => $name)),

    are you able to find the difference between the two code fragments?

    tnlone’s picture

    tnlone commented

    I get you didn't correctly understand how the function t() is used, and when

    I think you are misconstructing what I am trying to say...
    I think I know a bit about t() function after working with it for a while now.... I would make a reference to the Drupal API.

    Any text within t() can be extracted by translators and changed into the equivalent text in their native language.
    Special variables called "placeholders" are used to signal dynamic information in a string which should not be translated. Placeholders can also be used for text that may change from time to time (such as link paths) to be changed without requiring updates to translations.

    Also to make it clear the example as set in the Drupal API 

    $message[] = t("If you don't want to receive such e-mails, you can change your settings at !url.", array('!url' => url("user/$account->uid", array('absolute' => TRUE))));

    In my code 

      '#title' => t('!name', array('!name' => $name)),

    I am using place holder !name for dynamic display of the node names in the theme settings page, so where am I violating the Drupal Coding Standards, and if this very same statement is being used by other themes, why is my work being penalized for this.
    if I am wrong or lacking this knowledge can you please point me in the right direction.

    are you able to find the difference between the two code fragments?

    Well my point is both are using t() with placeholders to output a dynamic text string.

    Anonymous’s picture

    Anonymous (not verified) commented

    There is a difference between what Garland code is doing, and what your code does. What is the difference between the code you reported present in Garland, and the code you wrote?
    The reason I am asking this is that, noting the difference between the two code fragments, you would also notice why the code you wrote is not correct.

    tnlone’s picture

    tnlone commented
    StatusFileSize
    new drustart-6.x.tar_.gz185.15 KB

    Well I changed the the place-holder from !name to @name as per Dynamic or static links and HTML in translatable strings

      '#title' => t('!name', array('!name' => $name)),

    So the new code is written as following:

    '#title' => t('@name', array('@name' => $name)),
    Anonymous’s picture

    Anonymous (not verified) commented

    The type of placeholder used is not the point.

    tnlone’s picture

    tnlone commented
    Status: Needs work » Needs review

    The reason I am asking this is that, noting the difference between the two code fragments, you would also notice why the code you wrote is not correct

    I do understand your concern in this regard and the point you are making in the difference between two codes. However, as I said before my argument is based on the place holders for t().

    Well if there is any suggestion you have I am willing to incorporate it..

    tnlone’s picture

    tnlone commented

    The type of placeholder used is not the point.

    So what's the point can you please inform me... I would leave this application right now...and close the issue queue as I clearly do not have the clear understanding of the coding principles then. however it would be great to know what I did wrong

    [Edited by kiamlaluno to remove a sentence all in bold]

    avpaderno’s picture

    avpaderno
    he/him
    Primary language Italian
    Location Brescia, 🇮🇹 🇪🇺
    commented
    Status: Needs review » Needs work

    The point is simply this:

    The following code is correct

      return t('!datetime — !username',
    array(
      '!username' => theme('username', $comment),
      '!datetime' => format_date($comment->timestamp)
    ));

    while the following code fragments are not correct

      '#title' => t('!name', array('!name' => t($name))),

      '#title' => t('!name', array('!name' => $name)),

    Are you able to report why the code used by Garland is correct, and your is not? In other words, as the first is correct and the last is not correct, there must be a difference; are you able to see the difference?

    tnlone’s picture

    tnlone commented

    I think that was my last question to you guys...

    So what's the point can you please inform me... I would leave this application right now...and close the issue queue as I clearly do not have the clear understanding of the coding principles then. however it would be great to know what I did wrong

    As I said before I just want to know what's wrong with my code...that's it... To me my code is not wrong... Simple

    avpaderno’s picture

    avpaderno
    he/him
    Primary language Italian
    Location Brescia, 🇮🇹 🇪🇺
    commented

    To make a summary, you have been suggested to change the code because it was not correct, and you insist to say it is correct without to be able to say why you think it should be correct.

    Who translates the strings used in a module, when see a string like '!datetime — !username' can change it to '!username — !datetime' because in the translated language the preferred order is different, or can change the character used a separator ( the — character) because in the translated language the character used for that purpose is different; but when who translates finds a string like '!name', the string would be left as it is.

    tnlone’s picture

    tnlone commented
    Status: Needs work » Closed (fixed)

    I am closing this issue queue because of simple fact that I am being discriminated by the reviewers...

      '#title' => t('!name', array('!name' => $name)),

    According to them this code fragment is not acceptable in CVS... but I wonder if they have checked
    http://drupal.org/project/acquia_marina
    http://drupal.org/project/adaptivetheme

    and many more projects that have the very same fragment...

    I just don't want to dwell into this because of my principle...if it's ok for them to have it, it should be ok for me to have it... why is it only me who is asked to change it... Just because I am simple guy I guess...

    So good bye Drupal CVS and it's reviewers.

    avpaderno’s picture

    avpaderno
    he/him
    Primary language Italian
    Location Brescia, 🇮🇹 🇪🇺
    commented
    Status: Closed (fixed) » Closed (won't fix)

    Reviews are not thought to review the code of existing projects; they are thought to check if the applicant understood how a module code should be written.
    If the applicant reports that '#title' => t('!name', array('!name' => $name)) is equivalent of t('!datetime — !username', array('!username' => theme('username', $comment),'!datetime' => format_date($comment->timestamp))), then I take he doesn't have a good knowledge of the Drupal API.