Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add First, Last, Pos, TotalItems and EvenOdd helpers #282

Merged
merged 2 commits into from
Aug 2, 2018
Merged

Add First, Last, Pos, TotalItems and EvenOdd helpers #282

merged 2 commits into from
Aug 2, 2018

Conversation

wilr
Copy link
Member

@wilr wilr commented Jul 31, 2018

When in an Element template the expected $First, $Last helpers aren't there so this replaces them with something.

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added an idea.

Also note that we're in the process of updating the master branch for elemental v4 with some breaking changes. You may want to target the 3 branch

*/
public function isFirstElement()
{
return !($this->Parent()->Elements()->filter('Sort:LessThan', $this->Sort)->exists());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you reckon about $this->Parent()->Elements()->first() === $this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would $this be robust enough? I'm just thinking of objects perhaps changing at runtime with something like fluent etc..

Perhaps $this->Parent()->Elements()->first()->ID === $this->ID. ID is all that matters right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah give that a go. Your tests should be good enough to try it out =D

@robbieaverill
Copy link
Contributor

Related: #211

@wilr
Copy link
Member Author

wilr commented Jul 31, 2018

@robbieaverill What to you think like #211 should we do something at the ElementArea level or leave it as functions on BaseElement if we leave it as methods on BaseElement then perhaps it would be more expected behaviour that First, Last be the method names rather than isFirstElement

@robbieaverill
Copy link
Contributor

@wilr I'm happy to follow your judgement =) they sound like useful methods either way

@wilr wilr changed the base branch from master to 3 July 31, 2018 21:47
@wilr wilr changed the title Add isFirstElement, isLastElement and isOnlyElement helpers Add First, Last, Pos, TotalItems and EvenOdd helpers Jul 31, 2018
@wilr
Copy link
Member Author

wilr commented Jul 31, 2018

Ok my judgement is consistently probably out weighs risk since the behaviour for all the options is going to be the same unless you start to do funky things like building your own ArrayList of them. In which case you could override First, Last anyway if they didn't work.

Updated the code, tests and docs. Changed branch to 3

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor doc block comments

}

/**
* @return boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: should be int

/**
* Returns the position of the current element.
*
* @return boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int

}

/**
* @return boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

string

if ($this->hasExtension(Versioned::class)) {
$records = Versioned::get_by_stage(BaseElement::class, Versioned::DRAFT);
} else {
$records = BaseElement::get()->max('Sort');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review but it appears this is a bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ScopeyNZ What seems to be the bug? I added this in as it was skipping draft version numbers and it was also using the global max rather than the local max.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$records = ...->max('Sort') should return an int, but is treated like an object on the next line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, but only if versioned had been removed with is rare. I'll submit a fix to the 3 branch.

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants