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

feat: Update composer dependencies, add php-cs-fixer, phpstan with config #208

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

ptrcksc
Copy link

@ptrcksc ptrcksc commented Aug 16, 2024

  • Update composer dependencies to php 8.1 max.
  • Add php-cs-fixer config.
  • Add phpstan config.
  • Format code.
  • Create phpstan baseline, fix one broken return type.
  • Refactor wp.php into seperate classes.
  • Add typehints to properties in src/Blocks/
  • Format readme into new style.
  • Upgrade demo app to laravel 10 (php8.1 support).
  • Add image upload example to demo app.
  • add eslint config.
  • format js code.

@ptrcksc ptrcksc requested a review from dnwjn August 16, 2024 14:26
@ptrcksc ptrcksc marked this pull request as ready for review August 16, 2024 14:26
@ptrcksc ptrcksc requested review from dnwjn and removed request for dnwjn August 27, 2024 13:10
README.md Outdated Show resolved Hide resolved
README.md Outdated

## Quick start

### Installation

Install package using composer:
Copy link
Member

Choose a reason for hiding this comment

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

Composer capitalized please :)

Copy link
Member

Choose a reason for hiding this comment

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

And would it be good to mention the minimum requirements?

Copy link
Author

Choose a reason for hiding this comment

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

And would it be good to mention the minimum requirements?

I do not think so, the PHP requirement is set in the composer file: "php": "^8.1",

Copy link
Member

Choose a reason for hiding this comment

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

That's handy, but people first land on the readme, so if they want to know if their environment can work with this package they would need to check the Composer file, which requires extra clicks.

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
<a href="https://van-ons.nl">
<img src="https://van-ons.nl/assets/mail/logo-vo-groen-2019-mail.png"/>
</a>
<br><br><br>
Copy link
Member

Choose a reason for hiding this comment

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

This can go, as it's replaced with the one at the end.

Copy link
Member

Choose a reason for hiding this comment

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

The logo is still here.

composer.lock Outdated Show resolved Hide resolved
example/app/Http/Middleware/RedirectIfAuthenticated.php Outdated Show resolved Hide resolved
* @since 5.0.0
* @var int
*/
public $token_start;
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 should be camelCase, right?

Copy link
Member

Choose a reason for hiding this comment

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

The constructor parameters should also be camel case.

example/resources/js/app.js Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
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