Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Frameworks-compatible Breadcrumbs and Paginator #952

Closed
wants to merge 3 commits into from

4 participants

@planardothum

When using a CSS framework like Zurb foundation, or Twitter bootstrap, the HtmlHelp code can now be coaxed into generating compatible markup.

Added new Breadcrumb options for separator, firstClass, and lastClass in style similar to what was already there for paginator. Paginator will use <a> instead of span for current page. Wrap elements in span for styling.

planardothum added some commits
@planardothum planardothum Twitter Bootstrap Compatibility
Wrap inactive elements in span (current page, prev/next buttons at ends
of range).
b034d9d
@planardothum planardothum More Flexible Paginator
Zurb foundation expects pagination links wrapped in anchor tags <a>
even on the current page. This seems like a safer approach.
ed2f701
@planardothum planardothum Framework-compatible breadcrumbs
Add new options to getCrumbList to allow it to be used to make
breadcrumbs in Twitter Bootstrap or Zurb foundation. Follows the model
of PaginatorHelper
6f238a4
@rchavik
Collaborator

i like this idea, but since it introduces a new functionality it should go into 2.3 branch instead of master.

@markstory markstory commented on the diff
lib/Cake/View/Helper/HtmlHelper.php
@@ -688,6 +693,12 @@ public function getCrumbs($separator = '&raquo;', $startText = false) {
* @link http://book.cakephp.org/2.0/en/core-libraries/helpers/html.html#creating-breadcrumb-trails-with-htmlhelper
*/
public function getCrumbList($options = array(), $startText = false) {
+ $defaults = array('firstClass'=>'first', 'lastClass'=>'last', 'separator' => '');
@markstory Owner

I thought the default for separator was supposed to be &raquo;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff
lib/Cake/View/Helper/PaginatorHelper.php
((6 lines not shown))
} else {
unset($options['rel']);
- return $this->Html->tag($tag, $title, array_merge($options, compact('escape', 'class')));
+ return $this->Html->tag($tag, '<a>'.h($title).'</a>', array_merge($options, compact('class')));
@markstory Owner

You need spaces around .. This also introduces new markup for the current page which may break existing stylesheets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory markstory commented on the diff
lib/Cake/View/Helper/PaginatorHelper.php
@@ -714,7 +714,7 @@ public function numbers($options = array()) {
if ($class) {
$currentClass .= ' ' . $class;
}
- $out .= $this->Html->tag($tag, $params['page'], array('class' => $currentClass));
+ $out .= $this->Html->tag($tag, '<a>'.$params['page'].'</a>', array('class' => $currentClass));
@markstory Owner

More spaces needed here, and the same concerns with new markup causing issues in existing applications. Also you should HTML escape $params['page']. You can never be too safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory
Owner

I don't think this can be merged in as it stands there are going to be a pile of tests that fail because of the additional markup and other changes added. You'll need to update the test cases. Also since this is a new feature/enhancement the pull request should be targeted towards 2.3 not master.

@planardothum

Thanks for the feedback. I'll retarget this to 2.3 and look at updating the tests. I'll also explore some possible compromise markup in paginator so it won't break existing stylesheets.

@ADmad
Collaborator

Even if targeted towards 2.3 with all the extra markup and other changes I doubt if keeping backwards compatibility with 2.2 would be possible, which we try to maintain in all 2.x releases. I would rather have people using css frameworks extend and customize the helpers to their needs rather than these changes forcing existing users to update their code. We can surely address these issues in the next major release 3.0 where we don't have to worry about BC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 1, 2012
  1. @planardothum

    Twitter Bootstrap Compatibility

    planardothum authored
    Wrap inactive elements in span (current page, prev/next buttons at ends
    of range).
  2. @planardothum

    More Flexible Paginator

    planardothum authored
    Zurb foundation expects pagination links wrapped in anchor tags <a>
    even on the current page. This seems like a safer approach.
Commits on Oct 2, 2012
  1. @planardothum

    Framework-compatible breadcrumbs

    planardothum authored
    Add new options to getCrumbList to allow it to be used to make
    breadcrumbs in Twitter Bootstrap or Zurb foundation. Follows the model
    of PaginatorHelper
This page is out of date. Refresh to see the latest.
View
18 lib/Cake/View/Helper/HtmlHelper.php
@@ -681,6 +681,11 @@ public function getCrumbs($separator = '&raquo;', $startText = false) {
* similar to HtmlHelper::getCrumbs(), so it uses options which every
* crumb was added with.
*
+ * ### Options
+ * - 'separator' Separator content to insert in between breadcrumbs, defaults to '&raquo;'
+ * - 'firstClass' Class for wrapper tag on the first breadcrumb, defaults to 'first'
+ * - 'lastClass' Class for wrapper tag on current active page, defaults to 'last'
+ *
* @param array $options Array of html attributes to apply to the generated list elements.
* @param string|array|boolean $startText This will be the first crumb, if false it defaults to first crumb in array. Can
* also be an array, see `HtmlHelper::getCrumbs` for details.
@@ -688,6 +693,12 @@ public function getCrumbs($separator = '&raquo;', $startText = false) {
* @link http://book.cakephp.org/2.0/en/core-libraries/helpers/html.html#creating-breadcrumb-trails-with-htmlhelper
*/
public function getCrumbList($options = array(), $startText = false) {
+ $defaults = array('firstClass'=>'first', 'lastClass'=>'last', 'separator' => '');
@markstory Owner

I thought the default for separator was supposed to be &raquo;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ $options += $defaults;
+ $firstClass = $options['firstClass'];
+ $lastClass = $options['lastClass'];
+ $separator = $options['separator'];
+ unset($options['firstClass'], $options['lastClass'], $options['separator']);
$crumbs = $this->_prepareCrumbs($startText);
if (!empty($crumbs)) {
$result = '';
@@ -701,9 +712,12 @@ public function getCrumbList($options = array(), $startText = false) {
$elementContent = $this->link($crumb[0], $crumb[1], $crumb[2]);
}
if ($which == 0) {
- $options['class'] = 'first';
+ $options['class'] = $firstClass;
} elseif ($which == $crumbCount - 1) {
- $options['class'] = 'last';
+ $options['class'] = $lastClass;
+ }
+ if (!empty($separator) && ($crumbCount - $which >= 2)) {
+ $elementContent .= $separator;
}
$result .= $this->tag('li', $elementContent, $options);
}
View
8 lib/Cake/View/Helper/PaginatorHelper.php
@@ -471,10 +471,10 @@ protected function _pagingLink($which, $title = null, $options = array(), $disab
$url = array_merge(array('page' => $paging['page'] + ($which == 'Prev' ? $step * -1 : $step)), $url);
if ($this->{$check}($model)) {
- return $this->Html->tag($tag, $this->link($title, $url, array_merge($options, compact('escape', 'model'))), compact('class'));
+ return $this->Html->tag($tag, $this->link($title, $url, array_merge($options, compact('escape'))), compact('class'));
} else {
unset($options['rel']);
- return $this->Html->tag($tag, $title, array_merge($options, compact('escape', 'class')));
+ return $this->Html->tag($tag, '<a>'.h($title).'</a>', array_merge($options, compact('class')));
@markstory Owner

You need spaces around .. This also introduces new markup for the current page which may break existing stylesheets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
}
@@ -714,7 +714,7 @@ public function numbers($options = array()) {
if ($class) {
$currentClass .= ' ' . $class;
}
- $out .= $this->Html->tag($tag, $params['page'], array('class' => $currentClass));
+ $out .= $this->Html->tag($tag, '<a>'.$params['page'].'</a>', array('class' => $currentClass));
@markstory Owner

More spaces needed here, and the same concerns with new markup causing issues in existing applications. Also you should HTML escape $params['page']. You can never be too safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if ($i != $params['pageCount']) {
$out .= $separator;
}
@@ -747,7 +747,7 @@ public function numbers($options = array()) {
if ($class) {
$currentClass .= ' ' . $class;
}
- $out .= $this->Html->tag($tag, $i, array('class' => $currentClass));
+ $out .= $this->Html->tag($tag, "<a>$i</a>", array('class' => $currentClass));
} else {
$out .= $this->Html->tag($tag, $this->link($i, array('page' => $i), $options), compact('class'));
}
Something went wrong with that request. Please try again.