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 Prev/Next order button in the Order page #17752

Merged

Conversation

@tdavidsonas88
Copy link
Contributor

tdavidsonas88 commented Feb 20, 2020

Questions Answers
Branch? 1.7.7.x
Description? As a merchant, I want to go to the next order without leaving the page. On the action bar button, added the arrow next prev on the left
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #17648
How to test? select order and interact with next/prev buttons on the top right of the Order view page

This change is Reviewable

@tdavidsonas88 tdavidsonas88 requested a review from PrestaShop/prestashop-core-developers as a code owner Feb 20, 2020
@tdavidsonas88 tdavidsonas88 changed the title [ORDER PAGE] Prev/Next order button DRAFT: [ORDER PAGE] Prev/Next order button Feb 20, 2020

public function present(?int $orderId): array
{
$order = new Order($orderId);

This comment has been minimized.

Copy link
@matks

matks Feb 20, 2020

Contributor

What happens if you pass null to new Order() ? Nothing ?

@tdavidsonas88 tdavidsonas88 changed the title DRAFT: [ORDER PAGE] Prev/Next order button [ORDER PAGE] Prev/Next order button Feb 20, 2020
@tdavidsonas88

This comment has been minimized.

Copy link
Contributor Author

tdavidsonas88 commented Feb 20, 2020

finished with design:
image

@matks Ready for review!

@matks matks added the migration label Feb 20, 2020
Copy link
Contributor

jolelievre left a comment

I think this solution is overkill, we add a new layer of OrderPresenter, the solution we discussed about extending OrderForViewing seemed more straightforward
Besides in this cas you have to instanciate two Order object to fetch two IDs which also seems overkill

@tdavidsonas88 tdavidsonas88 requested review from Progi1984, matks and jolelievre Feb 20, 2020
@tdavidsonas88

This comment has been minimized.

Copy link
Contributor Author

tdavidsonas88 commented Feb 20, 2020

I think this solution is overkill, we add a new layer of OrderPresenter, the solution we discussed about extending seemed more straightforward
Besides in this cas you have to instanciate two Order object to fetch two IDs which also seems overkill

It seems like it's simpler to add params to GetOrderForViewing and use them without DataProvider. However OrderForViewing constructor is already very big and adding two extra params with null default values might just bring some more complexity to Behat tests. Both solutions are good, it would be nice if @matks and @jolelievre would stick with one of them!

@eternoendless eternoendless changed the title [ORDER PAGE] Prev/Next order button Add Prev/Next order button in the Order page Feb 20, 2020
@matks
matks approved these changes Feb 21, 2020
@Progi1984 Progi1984 added this to the 1.7.7.0 milestone Feb 21, 2020
@matks matks dismissed jolelievre’s stale review Feb 24, 2020

Jo is gone in holidays for 5 days :D so we cannot wait for his review

@eternoendless eternoendless linked an issue that may be closed by this pull request Feb 24, 2020
@matthieu-rolland matthieu-rolland merged commit 2f3033d into PrestaShop:1.7.7.x Feb 24, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.