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

Update to Grid docs #135

Merged
merged 5 commits into from Dec 12, 2018

Conversation

Projects
None yet
5 participants
@sarjon
Copy link
Member

sarjon commented Sep 10, 2018

No description provided.

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Sep 10, 2018

ping @mickaelandrieu for review

And finally register your Grid definition factory as a service.

```yaml
prestashop.core.grid.definition.factory.products_definition_factory:

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor
- prestashop.core.grid.definition.factory.products_definition_factory
+ prestashop.core.grid.definition.factory.product_grid_definition_factory
But in case you need to create Grid Definition by hand, here's how you can do that.

```php
$productsGridDefinitionFactory = $container->get('prestashop.core.grid.definition.factory.products_definition_factory');

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor

service name needs to be updated

## Grid Query Builder
{{% notice info %}}
Grid component itself does not manage Search Criteria but instead it provides interface for it.
In PrestaShop [Filters component](#) is used to resolve Search Criteria for Grid.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor

I need time to document it, would you mind to create an issue so I won't forget?

This comment has been minimized.

@jolelievre

jolelievre Sep 11, 2018

Contributor

Yeah I noticed this part was missing, as well as all the "Learn more" links
shouldn't we add "Coming soon" message? Are these pages supposed to be separate pages or sub sections of this one?
I wrote a little section about this in my PR which could be integrated until the full page is written
https://github.com/PrestaShop/docs/pull/134/files#diff-a04004808f283fab9e54e5fb0b03e318R238

This comment has been minimized.

@sarjon

sarjon Sep 11, 2018

Member

regarding "Learn more" i think those should be sub sections of Grid component, right?

public: true
```

Most of the time you wont be creating Grid Definition by yourself but delegating this task to other services.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor
- wont
+ won't

### Creating Grid Data

Grid component does not create Grid Data directly but instead relays on `PrestaShop\PrestaShop\Core\Grid\Data\Factory\GridDataFactoryInterface`.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor
- relays
+ relies?

ping @Quetzacoalt91 as my english is bad


But we got you covered, you can alter almost everything *before* the Grid is presented to the view thanks to the available hooks:
* [How to work with Bulk actions](#)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor

If you add these links, you need to complete them 👼

This comment has been minimized.

@sarjon

sarjon Sep 11, 2018

Member

can I add Comming soon and make different PR's for that?


{{< figure src="../img/grid_workflow_hooks.png" title="Grid Workflow" >}}
{{< figure src="../img/grid-build-schema.png" title="Grid schema" >}}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 10, 2018

Contributor

except if I've misread it, you have removed 1 schema: please let them both, as they have not the same utility: understand how it works vs describe the hooks.

This comment has been minimized.

@sarjon

sarjon Sep 11, 2018

Member

i guess you are right, but is it a correct place for it? I would prefer adding diagram with hooks to "How to modify Grid from modules" chapter as it's relevant to it, wdyt?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 13, 2018

Contributor

make sense 👍

@mickaelandrieu
Copy link
Contributor

mickaelandrieu left a comment

it's a good start, I have the feeling it's a wip

Show resolved Hide resolved src/content/1.7/development/components/grid-component.md Outdated
Show resolved Hide resolved src/content/1.7/development/components/grid-component.md Outdated
Show resolved Hide resolved src/content/1.7/development/components/grid-component.md Outdated
Show resolved Hide resolved src/content/1.7/development/components/grid-component.md Outdated
Show resolved Hide resolved src/content/1.7/development/components/grid-component.md Outdated
## Grid Query Builder
{{% notice info %}}
Grid component itself does not manage Search Criteria but instead it provides interface for it.
In PrestaShop [Filters component](#) is used to resolve Search Criteria for Grid.

This comment has been minimized.

@jolelievre

jolelievre Sep 11, 2018

Contributor

Yeah I noticed this part was missing, as well as all the "Learn more" links
shouldn't we add "Coming soon" message? Are these pages supposed to be separate pages or sub sections of this one?
I wrote a little section about this in my PR which could be integrated until the full page is written
https://github.com/PrestaShop/docs/pull/134/files#diff-a04004808f283fab9e54e5fb0b03e318R238

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Oct 19, 2018

I'm not done in the rebase yet, not really sure of the right structure of this part.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Oct 22, 2018

Hello @sarjon, would you mind to rebase your pull request?

Contents are organization of the main Grid page has changed a lot, I can't figure out what do I need to keep, adapt and remove 🤔

Thanks!

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Oct 22, 2018

ill take a look at this. :)

@sarjon sarjon force-pushed the sarjon:update-grid-docs branch from 0837648 to 82dff98 Oct 23, 2018

@@ -7,5 +7,7 @@ weight: 50
# Components

* [Grid Component][grid-component]
* [Faceted Search Component][faceted-search-component]

This comment has been minimized.

@sarjon

sarjon Oct 23, 2018

Member

it would be nice to separate FO and BO components, maybe with labels or separate sections, like FO components list and BO components list. :)

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Oct 23, 2018

@mickaelandrieu rebase done, but needs to be double checked.

by the way, when reading docs (and not only), i've noticed that "Grid actions" are not the best name (we already discussed it with @matks but didnt come up with new name 😞). I think that "Toolbar buttons/actions" would be better and easier to understand for PS developers since it's the same name as in legacy.

another thing is related to DataFactory, i would prefer DataProvider as its much easier to understand and gives you clear idea of what it does imo.

so, wdyt?

Show resolved Hide resolved src/content/1.7/development/components/grid/_index.md
_defaults:
public: true
When creating Search Criteria you can skip some or all it's data. You can leave default `null` values for order way
and order by to disable sorting or offset and limit to disable pagination.

This comment has been minimized.

@jolelievre

jolelievre Oct 23, 2018

Contributor

so offset has to be nullified to disable pagination? I would have thought nullifying limit is more logical

This comment has been minimized.

@sarjon

sarjon Dec 10, 2018

Member

if you want to disable pagination, you have to nullify both limit and offset, you can choose to disable one of them only (e.g. disable offset, but keep limit).

This comment has been minimized.

@jolelievre

jolelievre Dec 10, 2018

Contributor

I would rephrase this sentence, it's longer but clearer:

When creating Search Criteria you can skip some or all of its data. If you set both orderWay and orderBy to null it will disable sorting. If you set both offset and limit to null it will disable pagination.

Show resolved Hide resolved src/content/1.7/development/components/grid/_index.md
@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Nov 6, 2018

another thing is related to DataFactory, i would prefer DataProvider as its much easier to understand and gives you clear idea of what it does imo.

It's not a provider, but a factory and this make the architecture of the component inconsistent, so I disagree. It doesn't mean we can't invest in docs to better explain what is a factory to PrestaShop developers.

this class does not exist in 1.7.5, should we specify the minimum version to use this class?

No, the example should be updated using type hint on the interface instead

by the way, when reading docs (and not only), i've noticed that "Grid actions" are not the best name (we already discussed it with @matks but didnt come up with new name disappointed). I think that "Toolbar buttons/actions" would be better and easier to understand for PS developers since it's the same name as in legacy.

It's too late for that: we are too close from the release 1.7.5.0 stable release so the QA team won't accept such big changes on API which are not bug fixes and we will not create a BC on 1.7.6 => needs to be planned for 1.8.x

Yeah I noticed this part was missing, as well as all the "Learn more" links
shouldn't we add "Coming soon" message? Are these pages supposed to be separate pages or sub sections of this one?

Indeed, we should remove these links until they are complete 👍

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Dec 3, 2018

Hi @sarjon,

can you give to docs some love, please? We're really close to delivering 1.7.5 stable release and I've seen so many developers excited about the Grid component that we must provide to them the best docs ever, don't you think? /c if it's ok @matks?

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Dec 4, 2018

i wont be available much this week. :/

@sarjon sarjon force-pushed the sarjon:update-grid-docs branch from 82dff98 to dff1271 Dec 10, 2018

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Dec 10, 2018

@matks @mickaelandrieu latest comments addressed and rebased. 👍

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 10, 2018

ping @jolelievre 😉

_defaults:
public: true
When creating Search Criteria you can skip some or all it's data. You can leave default `null` values for order way
and order by to disable sorting or offset and limit to disable pagination.

This comment has been minimized.

@jolelievre

jolelievre Dec 10, 2018

Contributor

I would rephrase this sentence, it's longer but clearer:

When creating Search Criteria you can skip some or all of its data. If you set both orderWay and orderBy to null it will disable sorting. If you set both offset and limit to null it will disable pagination.

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 10, 2018

@sarjon only a last small change suggested, the rest si good for me as well

@matks
Copy link
Contributor

matks left a comment

With @jolelievre last comment it will be an approval for me 😉

@jolelievre
Copy link
Contributor

jolelievre left a comment

thanks @sarjon

@matks matks merged commit 6edaaf8 into PrestaShop:master Dec 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment