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

Migrate hooks for new Order view page #16144

Merged
merged 19 commits into from Dec 13, 2019
Merged

Conversation

@sarjon
Copy link
Contributor

sarjon commented Oct 28, 2019

Questions Answers
Branch? develop
Description? Migrates olds hooks from legacy page to new one. See details below
Type? improvement
Category? CO
BC breaks? yes, 2 hooks have been moved and renamed but aliases keep compatibility
Deprecations? no
Fixed ticket? Fixes #16523
How to test? See below

New hooks mapping

See below #16144 (comment)

How to test

  1. Build assets. Reinstall shop (SQL file modified).
  2. Go to modules/folder in CLI
  3. Copy test module there: git clone git@github.com:matks/orderhooks.git
  4. Go to BO "Module catalog" and search for "orderhook"
  5. Install "orderhook" test module
  6. Go to 1 order view page, example /admin-dev/index.php/sell/orders/orders/1/view
  7. See how hooks are displayed and compare to mapping
  8. Refresh. I added a "random 50%" to display the 2 usecases explained below

Known limitation

Mapping requires that "when there is not enough space for all buttons to be rendered, a dropdown list appears instead and buttons are put inside". I cant do that as my integration skills are very low.

What I did instead:

  • if 1 or 2 buttons are added (like module test) they are displayed
  • if more than 2 buttons are added, I display 1st button THEN next buttons are in dropdown (see screenshot below)

=> these 2 usecases can be seen with the test module, when you refresh there is a 50% luck you have usecase 1 or 2

Screenshots

Capture d’écran 2019-11-22 à 17 11 13
Capture d’écran 2019-11-22 à 17 11 21


This change is Reviewable

@sarjon sarjon marked this pull request as ready for review Oct 28, 2019
@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 28, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 28, 2019

Hi @TristanLDD, regarding "Print" button,

print

why is it highlighted as a hook? I could not find any hooks around it, does it have other meaning? :)

@sarjon sarjon added the migration label Oct 28, 2019
@MatShir

This comment has been minimized.

Copy link

MatShir commented Nov 6, 2019

Here the new hooks mapping! 🎉
New hooks

As discussed with @matks, we will add new hooks on the top with the other button. This hooks will force the module to have the same design as the other one.

Also, because there is no much space left and to not have lines of actions buttons, the ideal should be that it had them in a drop-list when there is not enough space.
Capture d’écran 2019-11-06 à 16 02 35

@MatShir MatShir removed the waiting for UX label Nov 6, 2019
@matks matks self-assigned this Nov 20, 2019
@matks matks force-pushed the sarjon:m/orders/hooks branch from 8cac0b2 to 3a93217 Nov 22, 2019
*
* @var string
*/
public $class;

This comment has been minimized.

Copy link
@matks

matks Nov 22, 2019

Contributor

Public properties to make them as easy to use as possible for modules

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Nov 25, 2019

Contributor

Use accessor instead public property

@@ -337,5 +337,14 @@
<alias>actionAfterDeleteProductInCart</alias>
<name>actionDeleteProductInCartAfter</name>
</hook_alias>

<hook_alias id="adminOrderLeft">

This comment has been minimized.

Copy link
@matks

matks Nov 22, 2019

Contributor

Hooks have moved on Edit an Order page so I gave them new name that suit better for their positioning. But I kept backward compatibility with hook aliases.

@matks matks dismissed their stale review via 27f59a7 Nov 22, 2019
@matks matks force-pushed the sarjon:m/orders/hooks branch from 2e4bee3 to 27f59a7 Nov 22, 2019
@jolelievre jolelievre force-pushed the sarjon:m/orders/hooks branch from e21e26b to 51af402 Dec 11, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 11, 2019

@sarahdib the PR was not ready for QA but it's no big deal ^^
The bug you found comes from the module which doesn't respect all the css integration rules but it's his responsibility so it doesn't affect the validity of this PR which is to add hooks at the expected places

However since I'm nice I made a few corrections on the module ^^ Here is the PR with the corrections matks/orderhooks#1

@matks
matks approved these changes Dec 12, 2019
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 12, 2019

@sarahdib Test module has been updated in order to allow you to validate this PR 😉

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Dec 13, 2019
@PierreRambaud PierreRambaud merged commit 0facef1 into PrestaShop:develop Dec 13, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Dec 13, 2019

Thanks everyone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.