Ensure default Post sort order shows newer Posts first #381
Conversation
| */ | ||
| public function findAll() | ||
| { | ||
| return $this->findBy(array(), array('publishedAt' => 'DESC')); |
There was a problem hiding this comment.
@simensen, you should use the short array syntax: [].
simensen
force-pushed
the
Post-sort-order
branch
from
5f2a425
to
6ede65b
Compare
| @@ -28,6 +28,14 @@ | |||
| class PostRepository extends EntityRepository | |||
| { | |||
| /** | |||
| * @return Post[] | |||
| */ | |||
| public function findAll() | |||
There was a problem hiding this comment.
Probably we should use a bit different method name like findAllOrdered() here instead of findAll() as we change its normal behavior from EntityRepository. What do you think?
There was a problem hiding this comment.
What about to call the findBy method instead of the findAll in the controller and don't change the findAll behavior at all?
There was a problem hiding this comment.
I'm fine either way and had considered these options. I figured it would be easier to submit a PR and start that discussion if someone objected to overriding findAll. :)
There was a problem hiding this comment.
Maybe something like findAllInChronologicalReversedOrder() (not sure if there is a better English term for this name).
There was a problem hiding this comment.
If we're going to call it at least in a few different places - the findAllOrdered() is more preferred than findBy(). Otherwise, I'm fine with findBy().
There was a problem hiding this comment.
@bocharsky-bw, for now we use the findAll only once in the admin controller.
https://github.com/symfony/symfony-demo/search?utf8=%E2%9C%93&q=findAll
There was a problem hiding this comment.
Overriding the builtin doctrine methods to change their behavior is a bad practice IMO, so you should use a different name
There was a problem hiding this comment.
Alright, I changed to using findBy in the controller.
simensen
force-pushed
the
Post-sort-order
branch
from
6ede65b
to
87584a6
Compare
|
I think that's the simplest solution 👍 |
|
@simensen thanks! I like this feature and the solution was very simlpe at the end. |
…simensen) This PR was merged into the master branch. Discussion ---------- Ensure default Post sort order shows newer Posts first In at least #223 the topic of sorting on the **admin backend** was mentioned. This is the first time I've loaded the demo app in quite awhile and wasn't sure it was working because I didn't realize the new post I had added was going to show up at the bottom of the list. It may not be a problem for everyone but the bottom of the list on my laptop was well past the fold. I appreciate that we do not need to make this **admin backend** fully completed with pagination and column-by-column sorting and what-not. However, I think it would make sense to have newer posts showing up at the top. Commits ------- 87584a6 Ensure default Post sort order shows newer Posts first
In at least #223 the topic of sorting on the admin backend was mentioned. This is the first time I've loaded the demo app in quite awhile and wasn't sure it was working because I didn't realize the new post I had added was going to show up at the bottom of the list. It may not be a problem for everyone but the bottom of the list on my laptop was well past the fold.
I appreciate that we do not need to make this admin backend fully completed with pagination and column-by-column sorting and what-not. However, I think it would make sense to have newer posts showing up at the top.