jun [jun]

subscribing

Hello, and thanks for applying for a CVS account.

TinyMCE is already supported by Wysiwyg (http://drupal.org/project/wysiwyg). Is there any particular reasons to create another module?

To clarify the confusion: http://css-tricks.com/better-pull-quotes/ is what this client-side editor plugin is about. You write some text as usual, then you select a phrase or sentence, you click the "Pullquote" editor button, and POOF!, that phrase is going to be duplicated and pulled into a nicely looking floating container next to the text.

Code review:

1) The permanently loaded JS should use a proper Drupal behavior.

2) Most of the code is duplicated from Wysiwyg's Teaser break plugin. It's hard to understand, which parts of the code actually belong to this plugin. Other code should be removed. The same applies to code comments, which do not explain anything currently.

3) JS and CSS namespaces should all be "pullquote", not "pulledquote".

4) The plugin language files (only valid for TinyMCE) register strings for a wrong plugin.

5) Make sure you follow Drupal's coding standards, also for JS and CSS: http://drupal.org/coding-standards

In short: This plugin module needs a massive clean-up. Additionally, but that's not really blocking this CVS application, I'm not sure whether it's a good idea to plant many small Wysiwyg 2.x plugins onto drupal.org, as the cross-editor API in 2.x is still weak. The API will hopefully improve soon and incarnate as 3.x, but until then, or perhaps even regardless of that, I'm a bit scared by scattering drupal.org with gazillions of small Wysiwyg plugin modules. I guess it would make more sense to build a single "Wysiwyg plugins" (wysiwyg_plugin) project, in which various plugins of this kind are contained and multiple maintainers care for bug reports, support requests, and feature requests, and also for incorporating new plugins. The only difference to a stand-alone module would be the namespace (wysiwyg_plugin_pullquote), and potentially, some plugins may even be registered through a plugin grouping module (though not necessarily this one, as you need to load JS/CSS on all pages). Lastly, a joint effort would lead to more collaboration among developers and maintainers, eventually also forming a common voice to raise against Wysiwyg core API development.

Thoughts?

I would love to see a working module that allows pull-quotes to be created in the way that Jun and http://css-tricks.com/better-pull-quotes/ before him envisage. I am not a programmer, so much of Sun's comments mean nothing to me (sorry). However, surely the way forward is to create a simple Drupal module that enables http://css-tricks.com/better-pull-quotes/ JQuery and javascript. The span tags <span class="pullquote">lorem ipsum</span> can be injected by manual typing.

If a webmaster wants to use markup editor such as MarkItUp, TinyMCE, or any other, this simply requires a separate and unconnected customisation of a command button.

So, am I right in thinking that Jun has unnecessarily complicated this by integrating the pullquote functionality with TinyMCE? I do not know how to create Drupal modules, but I wonder if my thoughts could help Jun or others to get this module off the ground? Thanks

There is "pullquote" which identify content that should be pulled. And once the jQeury pulls it, it's then added "pulledquote" which has the proper CSS formatting added.

In Drupal, this is normally done by using Drupal's JS behaviors, whereas each behavior may add a .pullquote-processed CSS class to the processed DOM elements. That makes you also stay within your namespace. You never know whether there'll be a pullledquote module/project on drupal.org someday.

Hi Jun and Sun

I am not a programmer. However, with a little help from a friend, last night I got a basic module working doing what I described in post 5 above. The rendered layout is a little messed-up, but I have CSS skills and will solve this over the next few days. As my first mini-module, I am very proud with this!

I'd like to:

1. add some functionality for adding a strap line for the author;
2. read-up on Drupal JS behaviours to see if I can work out how to adapt the JS as per Sun's suggestion;
3. add an option for the admin to change the CSS id name to whatever they like if this isn't too complicated and I can find other modules that do this sort of thing that I can look at to see if I can work out how to do this.

I hope you guys can assist if and when I get stuck (note my name!). If and when, I get this rudimentary (but complex for me) module working and up to to Drupal standards, I am happy to publish, but really would like someone who is more skilled at programming to be a co-maintainer, as I do not have the skills to do this on my own. Any offers?

FYI: @stuckagain applied for CVS in #898332: stuckagain [stuckagain]

@jun: Any updates or feedback from your side on #7 and #8?

I have uploaded my attempt to create this module as per Jun's suggestion at #6. See http://drupal.org/node/898332

Jun

That would be excellent. I have attached my latest at the code. I'd suggest we keep things simple to star with and put this core code up for CVS review. Then once we have passed that hurdle, the next step could be to adapt to work with wysiwg filters.

As the original CVS application, was yours, you will have to do this, as you would also have to approve me to co-maintain.

Thanks and over to you!

All minor stuff:

• Drupal.behaviors.myModuleBehavior = function (context) {
  
  		$('span.pullquote').each(function(index) { 
  			var $parentParagraph = $(this).parent('p'); 
  			$parentParagraph.css('position', 'relative'); 
  			$(this).clone() 
  			  .addClass('pulledquote') 
  			  .prependTo($parentParagraph); 
  		});
  		 
  		$('span.pullquoteopposite').each(function(index) { 
  			var $parentParagraph = $(this).parent('p'); 
  			$parentParagraph.css('position', 'relative'); 
  			$(this).clone() 
  			  .addClass('pulledquoteopposite') 
  			  .prependTo($parentParagraph); 
  		});
  };
  

  1. Wrong behavior name. ;)
  2. All jQuery selectors in Drupal behaviors should use the passed context variable.
  3. Each .each() callback should start by adding a .pullquote-processed CSS class to the SPAN. Behaviors are invoked multiple times and you do not want to do this repeatedly.
  4. The entire .pullquoteopposite looks redundant and needless to me. You want to use just one additional CSS class that changes the styling, but that's merely CSS, not even remotely touched by the JS. Since you clone the original SPAN, all of its classes are taken over, too. Thus, you can easily use a .pullquote-left class (and gazillions of other variants) to change the appearance. But again, that's plain CSS.
  5. Likewise, I don't understand why the JS changes the style to position: relative. That's a job for CSS, not JS.
  6. The index variables are unused.
  7. We do not use tabs anywhere in Drupal. Always two spaces. And no trailing white-space.
  8. You want to follow Drupal's JS coding standards: http://drupal.org/node/172169

  Overall, you likely want just this:

  Drupal.behaviors.pullquote = function (context) {
    $('span.pullquote:not(.pullquote-processed)', context).each(function () {
      var $span = $(this).addClass('pullquote-processed');
      // For now, paragraphs only. May be extended later on. (simply add ',div')
      var $parent = $span.parent('p');
      if ($parent.length) {
        // Apply conditional pullquote container styling.
        $parent.addClass('pullquote-container');
        $span.clone()
          .addClass('pullquote-quote')
          .prependTo($parent);
      }
    });
  };
  

  ...and:

  .pullquote-container {
    position: relative;
  }
  
  .pullquote-quote {
  	border: 1px solid #aaa;
  	float: right;
  	width: 30%;
  	background: #e8e8e8;
  	color: #333;
  	font: italic 1em Georgia, "Times New Roman", Times, serif;
  	line-height: 1.7em;
  	padding: 1em;
  	margin: 1em 0 0.4em 1em;
  }
  
  .pullquote-quote.pullquote-left {
    float: left;
    margin-left: 0;
    margin-right: 1em;
  }
  

• Not sure why you're hiding pullquotes in the print output. I'd imagine they look especially good there.

• This module does not change the database, so you do not need to run update.php
  

  Please never state something like this. Users should always visit update.php without exceptions.

• The .info file is partially using Windows AND Unix line endings. All files in Drupal are using Unix line endings only - unfortunately, this cannot be enforced on the server/repository side, so it's your job to ensure that all files are using

  - Unix line endings (LF only)

  - UTF-8 without Unicode signature (BOM)

• -- CONFIGURATION --
  
  None is necessary, unless you want to change the look of the pullquote to match your site,
  in which case, tyle the pullquote according to your requirements by editting the file pullquote.css.  
  

  You probably want to skip (remove) the entire section instead.

  Rule of thumb: Never ever write something that no one wants to read. If people get tired of reading, they won't RTFM anymore.

• /**
  * Simply includes a CSS and JS file
  */
  

  Wrong indentation of phpDoc block. See http://drupal.org/node/1354 for details.

•   drupal_add_css(drupal_get_path('module', 'pullquote') . '/pullquote.css');
    drupal_add_js(drupal_get_path('module', 'pullquote') . '/pullquote.js');
  

  You can prepare $path = drupal_get_path(...); once for both lines.

• The .info file should not specify package (if it is "Other", that's the default) and no version.

I'll approve this application + stuckagain's now. Happy co-maintaining, friends! :)

Sun.

Thanks for your help and skills. You have certainly helped beyond my skills. I have made all the changes and these are attached. I also worked out that my work does not comply with Drupal CSS standards - but it does now. Just one issue is outstanding. I couldn't work out how to follow Sun's recommendation to (sorry, I am not really a programmer).

You can prepare $path = drupal_get_path(...); once for both lines.

<?php
  drupal_add_css(drupal_get_path('module', 'pullquote') . '/pullquote.css');
  drupal_add_js(drupal_get_path('module', 'pullquote') . '/pullquote.js');
?>

Can anyone help me here?

Almost there!

That's:

  $path = drupal_get_path('module', 'pullquote');
  drupal_add_css($path . '/pullquote.css');
  drupal_add_js($path . '/pullquote.js');

This application has already been approved. Please go ahead and create your pullquote project. :)

Hi Jun

I am delighted to say that with Sun's kind help, I have published the new Pullquote module and added you as a co-maintainer. If you get a chance do please try things out and let me know what you think.

Next I plan to do some reading on D7, to see what - if anything - needs doing to release a D7 compatible version of this module. I also want to play around with the CSS a bit as I have spotted one or two instances when the CSS doesn't behave consistently.

I hope we will be able to work together to develop this module further.

I have tried to port the previously accepted pull-quote module for Drupal 6 to Drupal 7. Feedback welcomed on whether this is good enough to upload to the module's CVS for wider use.

Thanks

See #17

This application has been already approved; the review has been already done.

Yes, please continue development with new issues + hopefully an increasing user as well as developer base in your project's issue queue: http://drupal.org/project/issues/pullquote

"From here on, you are on your own" ;) Cold water. heheheh

Lastly, I hope both of you discussed the project's maintenance openly? While @jun seems to have CVS write access, he doesn't have other permissions. That's totally fine, as long as you both agreed on that. Happy to have the discussion here, if you still need to talk. Otherwise, happy co-maintaining! :)

Thanks Sun. Can I say a big thank you for your hand-holding. It has been helpful and very generous. As a newbie,it has really helped. I have emailed Jun without any reply. The CVS permissions was an error on my part, which I have since rectified and he has full access. :.)

I am giving credits to the users who participated in this issue.