[WIP] Pagination example with knplabs/knp-pagination-bundle#22
[WIP] Pagination example with knplabs/knp-pagination-bundle#22DaniCristante wants to merge 24 commits intoRunroom:masterfrom
Conversation
| $this->repository = $this->prophesize(BookRepository::class); | ||
|
|
||
| $this->service = new BookService($this->repository->reveal()); | ||
| $this->service = new BookService($this->repository->reveal(), new Paginator()); |
There was a problem hiding this comment.
Tengo dudas sobre modificar los tests, entiendo que debería modificarlos para que se adapten al nuevo código ¿verdad?, pero ahora el BookService espera un nuevo param, un PaginatorInterface. Al no poder instanciar una interfaz, ¿Entiendo que le debería pasar un new Paginator que es el que implementa la interfaz?
There was a problem hiding this comment.
Si es posible pasarle una clase real, mejor, sino tmb puedes mockerla. En el caso de dependencias externas (como es tu caso) mejor como lo estás haciendo
Codecov Report
@@ Coverage Diff @@
## master #22 +/- ##
============================================
- Coverage 51.43% 48.64% -2.79%
- Complexity 127 135 +8
============================================
Files 23 24 +1
Lines 488 518 +30
============================================
+ Hits 251 252 +1
- Misses 237 266 +29
Continue to review full report at Codecov.
|
DaniCristante
force-pushed
the
okr/pagination-example
branch
from
d07aa84 to
753f362
Compare
| ->add('_action', 'actions', [ | ||
| 'actions' => [ | ||
| 'delete' => [], | ||
| 'edit' => [], |
There was a problem hiding this comment.
Yo este cambio lo desharía, no es consistente con el resto de admins.
|
|
||
| public function list(Request $request): Response | ||
| { | ||
| $page = (int) $request->get('page', 1); |
There was a problem hiding this comment.
creo que si haces: $request->query->getInt('page', 1); te da directamente un int, puede ser?
| ); | ||
| } | ||
|
|
||
| public function list(Request $request): Response |
There was a problem hiding this comment.
porqué no usas el mismo método que ya teniamos de books() y amplias su funcionalidad para incluir esto?, así evitamos tener 2 listados.
| return $model; | ||
| } | ||
|
|
||
| public function getBooksListViewModel(int $page): BooksListViewModel |
There was a problem hiding this comment.
Aquí lo mismo, podríamos reusar el método ya existente: getBooksViewModel()
| use Knp\Bundle\PaginatorBundle\Pagination\SlidingPaginationInterface; | ||
| use Runroom\SamplesBundle\BasicEntities\Entity\Book; | ||
|
|
||
| class BooksListViewModel |
There was a problem hiding this comment.
Podríamos reusar el viewModel ya existente.
| path: /book/{slug} | ||
| controller: Runroom\SamplesBundle\BasicEntities\Controller\BookController::book | ||
|
|
||
| runroom_samples.basic_entities.books_list: |
There was a problem hiding this comment.
Podríamos reusar la url ya existente.
|
Cierro este PR, he abierto uno nuevo: #48 |
2 participants
No description provided.