Skip to content

API Added getBreadcrumbItems to SiteTree #974

Merged
merged 1 commit into from Jun 2, 2014

6 participants

@micmania1
SilverStripe Ltd. member

I've re-added the $unlinked functionality but changed this to $linked instead. The double negative isn't a very nice read. If you'd prefer I change this back let me know.

@chillu chillu commented on an outdated diff Mar 25, 2014
code/model/SiteTree.php
* @param boolean|string $stopAtPageType ClassName of a page to stop the upwards traversal.
* @param boolean $showHidden Include pages marked with the attribute ShowInMenus = 0
* @return HTMLText The breadcrumb trail.
*/
- public function Breadcrumbs($maxDepth = 20, $unlinked = false, $stopAtPageType = false, $showHidden = false) {
+ public function Breadcrumbs($maxDepth = 20, $linked = true, $stopAtPageType = false, $showHidden = false) {
@chillu
SilverStripe Ltd. member
chillu added a note Mar 25, 2014

We can't really change the semantics of this method. I agree its an awkward parameter naming, but its going to cause too much upgrade hassles, since we can't throw deprecation warnings when using the booleans "the other way around"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chillu chillu and 2 others commented on an outdated diff Mar 25, 2014
templates/BreadcrumbsTemplate.ss
@@ -1,5 +1,12 @@
<% if $Pages %>
<% loop $Pages %>
- <% if $Last %>$MenuTitle.XML<% else %><a href="$Link" class="breadcrumb-$Pos">$MenuTitle.XML</a> &raquo;<% end_if %>
@chillu
SilverStripe Ltd. member
chillu added a note Mar 25, 2014

This was kept on a single line to avoid rendering gaps when floating elements in IE, IIRC. Hopefully browsers have evolved since then though, and if you need to support ancient browsers there's always the option to override this template hehe

@kinglozzer
SilverStripe Ltd. member
kinglozzer added a note Mar 25, 2014

Probably because of whitespace with inline-block. Easiest cross-browser solution I've found (though certainly not the prettiest) is to wrap all the white space in HTML comments: https://gist.github.com/kinglozzer/9761228. But as you say, people can override the template if they encounter this anyway.

@micmania1
SilverStripe Ltd. member
micmania1 added a note Mar 25, 2014

For the sake of it, i'll just put it back to one line and add a comment so people know why its done that way. I'm sure the world will keep spinning (hopefully) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chillu
SilverStripe Ltd. member
chillu commented Mar 25, 2014

Other than the $unlinked thing, happy with this change. Build failure seems unrelated (https://travis-ci.org/silverstripe/silverstripe-cms/jobs/21358432)

@simonwelsh simonwelsh added the master label Mar 26, 2014
@micmania1
SilverStripe Ltd. member

bump ^

@tractorcow tractorcow commented on an outdated diff May 6, 2014
code/model/SiteTree.php
* @param boolean|string $stopAtPageType ClassName of a page to stop the upwards traversal.
* @param boolean $showHidden Include pages marked with the attribute ShowInMenus = 0
* @return HTMLText The breadcrumb trail.
*/
public function Breadcrumbs($maxDepth = 20, $unlinked = false, $stopAtPageType = false, $showHidden = false) {
+ $pages = $this->getBreadcrumbItems($maxDepth, $stopAtPageType, $showHidden);
+ $template = new SSViewer('BreadcrumbsTemplate');
+ return $template->process($this->customise(new ArrayData(array(
+ 'Pages' => $pages,
+ "Uninked" => $unlinked
@tractorcow
tractorcow added a note May 6, 2014

"Uninked" seems to have a spelling mistake

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

Restarted the build and it seems to be passing now.

Please rebase and fix the above and we can merge this. :)

@micmania1
SilverStripe Ltd. member

Fixed and rebased :)

@tractorcow

@ingo coding conventions wise, should we say that methods other than basic getters and setters should not be prefixed with get or set? I know we haven't been very consistent with this in the past though. Just one final thought before merging. :)

Probably something to go into coding-conventions.md. There is a part that says you can use one or the other, but not any best practice as to which is appropriate in which case.

@ingo
ingo commented May 8, 2014

@tractorcow FYI, I am likely a different ingo than the one you mean, so you may want to to ping a different person if you need an answer quickly :)

@tractorcow

Too late, you're a silverstripe dev now @ingo. We are all listening with baited breath to your thoughts on our coding conventions.

@tractorcow

I ment to ping @chillu ingo :)

@micmania1
SilverStripe Ltd. member

Anything more on this?

@tractorcow tractorcow merged commit cba0061 into silverstripe:master Jun 2, 2014
@micmania1 micmania1 deleted the micmania1:930-added-breadcrumbs-getter branch Jun 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.