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

[v4] Update docs #658

Merged
merged 33 commits into from
Feb 16, 2024
Merged

[v4] Update docs #658

merged 33 commits into from
Feb 16, 2024

Conversation

Mohammad-Alavi
Copy link
Contributor

@Mohammad-Alavi Mohammad-Alavi commented Feb 10, 2024

I just started using this awesome package and I thought It might be a good idea to fix/improve the docs as a way to contribute back while I am at it.
Ill mark it as ready when I can't find the time to continute working on it or I reach the end of the docs!

@Mohammad-Alavi Mohammad-Alavi changed the title Update docs Update v4 docs Feb 10, 2024
Comment on lines -235 to +240
SongData::collect(Song::all()); // returns a DataCollection of SongData objects
SongData::collect(Song::paginate()); // returns a PaginatedDataCollection of SongData objects
SongData::collect(Song::cursorPaginate()); // returns a CursorPaginatedCollection of SongData objects
SongData::collection(Song::all()); // returns a DataCollection of SongData objects
SongData::collection(Song::paginate()); // returns a PaginatedDataCollection of SongData objects
SongData::collection(Song::cursorPaginate()); // returns a CursorPaginatedCollection of SongData objects
Copy link
Contributor Author

@Mohammad-Alavi Mohammad-Alavi Feb 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used the previous versions of this pacakge, so I am not sure if this change is correct or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally correct!


```json
{
"type": "\\App\\Data\\CdRecordConfig",
"value": {
"data": {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the source code and it seems the data object value will be stored under the data key and not the value.
Please correct/revert this if I am wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@Mohammad-Alavi Mohammad-Alavi marked this pull request as ready for review February 11, 2024 11:49
@Mohammad-Alavi Mohammad-Alavi changed the title Update v4 docs [v4] Update docs Feb 11, 2024
Comment on lines -94 to +99
public static function prepareForPipeline(Collection $properties) : Collection
public static function prepareForPipeline(array $properties): array
{
$properties->put('metadata', $properties->only(['release_year', 'producer']));

return $properties;
$collection = collect($properties);
$collection->put('metadata', $collection->only(['release_year', 'producer']));

return $collection->toArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @alexrififi did it much better with his PR.
So maybe merge this PR first and then merge his PR, so it override this one.

I know it is much easier to change it here, but I didn't change it here because it felt like stealing 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know thanks!

Copy link
Member

@rubenvanassche rubenvanassche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, some small notes. Thanks!

@@ -63,4 +63,6 @@ class ArrayableNormalizer implements Normalizer
}
```

Normalizers are executed the order as they are defined in the `normalize` method. The first normalizer not returning null will be used to normalize the payload. Magical creation methods always have precedence on normalizers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these on one line, the spate docs site will make sure they're readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 💪

@@ -128,7 +129,9 @@ Since the collection type here is a `Collection`, the package will automatically

## DataCollection's, PaginatedDataCollection's and CursorPaginatedCollection's

The package also provides a few collection classes which can be used to create collections of data objects, it was a requirement to use these classes in the past versions of the package when nesting data objects collections in data objects. This is no longer the case and there are still valid use cases for them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these on one line, the spate docs site will make sure they're readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 😎

@@ -30,7 +15,11 @@ Or transform a data object to an array:
SongData::from(Song::first())->toArray();
```

By default, calling `toArray` on a data object will transform all properties to an array. This means that nested data objects and collections of data objects will also be transformed to arrays. Other complex types like `Carbon`, `DateTime`, `Enums`, ... will be transformed into a string. We'll see in the [transformers](/docs/laravel-data/v4/as-a-resource/transformers) section how to configure and customize this behavior.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these on one line, the spate docs site will make sure they're readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@@ -129,4 +118,5 @@ class AlbumData extends Data
}
```

As always, don't forget to type collections of data objects by annotation or the `DataCollectionOf` attribute, this is essential to transform these collections correctly.
As always, remember to type collections of data objects by annotation or the `DataCollectionOf` attribute,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep these on one line, the spate docs site will make sure they're readable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 🕺

Comment on lines -6 to -20
A data object can automatically be transformed into an array as such:

```php
SongData::from(Song::first())->toArray();
```

Which will output the following array:

```php
[
'name' => 'Never gonna give you up',
'artist' => 'Rick Astley'
]
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is removed due to being repeated on line 30 but let's keep this introduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rubenvanassche Yep, exattly. As it will be repeated literraly right after it. So you can count the next one as the intro too 👀
Just give the last word on reverting or keeping this change and I'll update it accordingly. 💪
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it as is, I'll fix it after merging the PR

@rubenvanassche
Copy link
Member

Good PR, thanks!

@rubenvanassche rubenvanassche merged commit 1bbfec6 into spatie:main Feb 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants