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

Preview extension for Grids #14682

Merged
merged 45 commits into from Oct 21, 2019

Conversation

@sarjon
Copy link
Contributor

sarjon commented Jul 16, 2019

Questions Answers
Branch? develop
Description? New orders list will require "Preview" feature of products, this PR introduces it
Type? new feature
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #14732
How to test? ⚠️ Build assets before testing ⚠️ When hover over row in new orders list you should see "drop-down" icon next to id, which after clicking loads order preview. List is accessible from /admin-dev/index.php/sell/orders/orders URL.

This change is Reviewable

@sarjon sarjon requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 16, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 16, 2019

@matks here you go. :) This is main component for Preview, we can improve UI later on.

@matks matks added the migration label Jul 17, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 18, 2019

What the ..., another random build failure. 👐

@matks matks mentioned this pull request Jul 18, 2019
7 of 41 tasks complete
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jul 24, 2019

@sarjon Nice !
Now comes the question: how do we handle the content shown in the preview 🤔.

The most simple idea seems to create a dedicated controller action, triggered by an ajax call, that returns some HTML. But it's resource-consuming as each click on "preview" will perform an ajax call...
Another good point for this idea: very easy to modify/extend the behavior.

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 24, 2019

I think that having endpoint like /orders/1/preview which returns JSON would be the best. It's totally decoupled from list and can be used elsewhere if needed.

But it's resource-consuming as each click on "preview" will perform an ajax call...

Hmm, I really don't think it's a problem, it's just an ajax, like any other. 🤷‍♂

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jul 25, 2019

It's supposed to be a "quick preview" and it's useful only if it's faster to use than loading the whole page 😅 so it needs to be quick enough to provide added value

Why return JSON ? Why not html to put in the modal ?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jul 25, 2019

Why return JSON ? Why not html to put in the modal ?

Because it's harder to customize. What if you want to return more than one html content?
It's really easier to do

{
  "res_products": "<div ....",
  "res_categories": "<div ..."
}

Than a single HTML block.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jul 25, 2019

Why return JSON ? Why not html to put in the modal ?

Because it's harder to customize. What if you want to return more than one html content?
It's really easier to do

{
  "res_products": "<div ....",
  "res_categories": "<div ..."
}

Than a single HTML block.

But then you must have some JS-side rendering 🤔 is'nt it a bit overcomplicated for our usecase ?

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Jul 25, 2019

Why return JSON ? Why not html to put in the modal ?

Because it's not a modal in the first place. 😄

We wouldn't load whole order information, only data that is needed for preview. I've seen poorly written modules do that, and they don't have any performance issues.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jul 25, 2019

Why return JSON ? Why not html to put in the modal ?

Because it's harder to customize. What if you want to return more than one html content?
It's really easier to do

{
  "res_products": "<div ....",
  "res_categories": "<div ..."
}

Than a single HTML block.

But then you must have some JS-side rendering is'nt it a bit overcomplicated for our usecase ?

Not really, many modules can add a hook to add data in the returns value, and populate whatever they want. It's really easier than doing another ajax request.
I'm just thinking about modules whose want to add modal, zoom, or many other information in the preview block (or outside this one).

@zuk3975 zuk3975 force-pushed the sarjon:preview-grid-column branch from 8f5510f to 1fa5fcb Sep 24, 2019
$.ajax({
url: this.previewDataUrl,
method: 'GET',
dataType: 'html',

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Sep 24, 2019

Contributor

😱
No possibility to get a json with exploitable data?

This comment has been minimized.

Copy link
@sarjon

sarjon Sep 24, 2019

Author Contributor

What do you mean? Instead of HTML we should get JSON data and render template from JS?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Sep 24, 2019

Contributor

Yes, and because if a module add something in its template and json, he can automatically add data (maybe we should also trigger js events here)

This comment has been minimized.

Copy link
@sarjon

sarjon Sep 24, 2019

Author Contributor

Yeah, here are 2 ways I can see:

  1. Get HTML from ajax, in that case we could dispatch hook to allow modules to add custom content to final template.

  2. Get JSON data from ajax, but in that case, we would need to render template in JS. It could be a bit more difficult for both us and module developers, because 1. We would need to manage translations in JS 2. Complex template rendering in JS would be harder than in Twig 3. It's harder for module developers to deal with JS event and adding custom content in JS than to execute hook when rendering from backend.

What do you think @PierreRambaud?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Sep 24, 2019

Contributor
  1. Get JSON data from ajax with {"htmlContent": "blablabl", "myCompanyHook": "Blabalbal"} and add a prestashop.emit('onOrderPreviewDataUrlSomething', {eventType: 'blabla', resp: resp}. More because you maybe wanted to execute or render something elsewhere the BO is rendering the htmlContent. So you can keep the rendering with PHP (if this one is too complicated to render with vuejs or jquery), and also trigger a custom event for modules.
@sarjon sarjon changed the title Preview extension for Grids [WIP] Preview extension for Grids Sep 24, 2019
@zuk3975 zuk3975 force-pushed the sarjon:preview-grid-column branch from a80b4f6 to 7aa620e Sep 26, 2019
@zuk3975 zuk3975 force-pushed the sarjon:preview-grid-column branch from 2a55cbb to d86a675 Sep 27, 2019
@tomas862

This comment has been minimized.

Copy link
Member

tomas862 commented Sep 27, 2019

todo:

  1. double click loads two previews
  2. loading animation somewhere?
@sarjon sarjon force-pushed the sarjon:preview-grid-column branch from 37bef00 to 8a1717a Sep 30, 2019
@sarjon sarjon changed the title [WIP] Preview extension for Grids Preview extension for Grids Oct 1, 2019
@sarjon sarjon requested a review from matks Oct 1, 2019
sarjon and others added 2 commits Oct 16, 2019
…eview.html.twig

Co-Authored-By: LouiseBonnard <32565890+LouiseBonnard@users.noreply.github.com>
@sarjon sarjon force-pushed the sarjon:preview-grid-column branch from 7b1e404 to 91b5bfb Oct 21, 2019
@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 21, 2019

It's ready once again. 🚀

@matks matks added this to the 1.7.7.0 milestone Oct 21, 2019
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Oct 21, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 21, 2019

Hi @sarjon !

I've noticed two defects on this PR :

1/ The Shipping and Invoice Adress are missing some fields (postal code, city, VAT Number, and Company name).
See screenshot :
PR14682 missing address info

It should be as in the order page :
PR14682 missing address info - Legacy 1

2/ On product list, there should be an indication of total number of product (see the invision screen) :
PR14682 missing products quantity Legacy 1

Otherwise, it appears good to me ! You should have soon a mail from the UX for some minors bugs on this page.

😄

@sarjon

This comment has been minimized.

Copy link
Contributor Author

sarjon commented Oct 21, 2019

Hi @Robin-Fischer-PS,

I've added missing fields. :)

@matks
matks approved these changes Oct 21, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 21, 2019

Thanks @sarjon ! It's now QA Approved 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 21, 2019

As Louise has already reviewed this PR (she has not yet given the seal of approval because she has not validated the changes yet) and in order to avoid this PR to need rebases again, I merge it but we're listening to feedbacks if there is 😉and we'll handle them in another PR

@matks matks merged commit 7c373b6 into PrestaShop:develop Oct 21, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 23 files, 27 discussions left (eternoendless, LouiseBonnard, MatShir, PierreRambaud, Robin-Fischer-PS, sarjon)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@sarjon sarjon deleted the sarjon:preview-grid-column branch Oct 21, 2019
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.