Skip to content

Conversation

shanerbaner82
Copy link
Contributor

No description provided.

Copy link
Member

@PeteBishwhip PeteBishwhip left a comment

Choose a reason for hiding this comment

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

Just a few comments and questions. Overall looks good. The URL's in deep-links are my biggest concern. Perhaps we should be using example.net instead?

### Laravel
#### For Windows
```shell
set JAVA_HOME=C:\Program Files\Microsoft\jdk-17.0.8.7-hotspot
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this can be replaced with a programatic approach? I'm not sure everyone will have 17.0.8.7 specifically? Not a blocker, just an observation.

Is it possible this isn't required if JAVA_HOME is already set in the Windows Env Variables?


For example:
```dotenv
https://bifrost.tech/property/456
Copy link
Member

Choose a reason for hiding this comment

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

Where did this URL come from? Accessing it shows "Domain for Sale".

Dialog::alert('Title', 'Message', $buttons, fn ($selected) => {
echo "You selected {$buttons[$selected]}";
});
Dialog::alert('Title', 'Message');
Copy link
Member

Choose a reason for hiding this comment

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

Have we dropped support for buttons on the alerts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonhamp and I are still discussing the best way vs the immediate way forward on this one.

SRWieZ
SRWieZ previously requested changes Apr 29, 2025
Copy link
Member

@SRWieZ SRWieZ left a comment

Choose a reason for hiding this comment

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

Looking good!

I added some notes here and there ^^


NativePHP for mobile is built to work with Laravel. You can install it into an existing Laravel application, or
[start a new one](https://laravel.com/docs/installation). The most painless way to get PHP and Node up and running on your system is with
[Laravel Herd](https://herd.laravel.com). It's fast and free!

## Private package
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 I will merge the Private package and Install NativePHP for mobile sections.

After running:

```bash
php artisan native:install
Copy link
Member

Choose a reason for hiding this comment

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

Since it references the command and we don't need to run it in this part of the documentation. I will use an inline code block instead of a full code block.

@@ -21,35 +21,35 @@ about whether a particular feature is specific to iOS or Android.

Most of the system-related features are available through the `System` facade.

## Synchronous vs. Asynchronous Methods
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure Synchronous vs. Asynchronous Methods belongs here. Maybe it should have its own page, even if it's a short one.

@@ -58,11 +58,13 @@ You may vibrate the user's device by calling the `vibrate` method:
System::vibrate()
```

_Coming Soon_ Options: `duration` and `intensity`

## Push Notifications
Copy link
Member

Choose a reason for hiding this comment

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

Push notifications should have their own dedicated page, and should contain an installation and configuration guide for FCM and APN.

4. An active [Apple Developer account](https://developer.apple.com/)
5. [A NativePHP for iOS license](https://checkout.anystack.sh/nativephp-ios)
6. _Optional_ iOS device
3. [A NativePHP for mobile license](https://checkout.anystack.sh/nativephp-ios)
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible wrong link also as mentioned by Pete

@@ -81,29 +83,6 @@ if ($token = System::getPushNotificationsToken()) {
Once you have the token, you may use it from your server-based applications to trigger Push Notifications directly to
Copy link
Contributor

Choose a reason for hiding this comment

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

This raised a couple of questions:

  • Should these tokens be stored on the server?
  • Could a user (on the server) have multiple tokens associated with it if they use multiple devices?
  • How to handle tokens when a user logs out and logs back in on with a different account on the device?

Can you give a brief explainer on what's involved to handle messages server side?

@gwleuverink
Copy link
Contributor

Oh boy just a couple more days guys! This looking great already

Copy link
Member

@PeteBishwhip PeteBishwhip left a comment

Choose a reason for hiding this comment

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

I'm good with it as-is with one comment on use of Windows in a comment. But thats not a blocker in my opinion at all. Nice work Shane.


#### For macOS
```shell
export JAVA_HOME=$(/usr/libexec/java_home -v 17) // This isn't required if JAVA_HOME is already set in the Windows Env Variables
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export JAVA_HOME=$(/usr/libexec/java_home -v 17) // This isn't required if JAVA_HOME is already set in the Windows Env Variables
export JAVA_HOME=$(/usr/libexec/java_home -v 17) // This isn't required if JAVA_HOME is already set in your system Environment Variables.

I believe it's the same for Mac, so likely no need to reference Windows specifically. :)

@shanerbaner82 shanerbaner82 dismissed stale reviews from gwleuverink and SRWieZ April 30, 2025 19:29

Covered

Copy link
Member

@simonhamp simonhamp left a comment

Choose a reason for hiding this comment

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

I made a few minor edits to clean some stuff up a bit, but otherwise this is looking great! Awesome work @shanerbaner82 🙌🏼

SHIP IT

@shanerbaner82 shanerbaner82 merged commit 23fc2a2 into main Apr 30, 2025
1 check passed
@shanerbaner82 shanerbaner82 deleted the v1-docs branch April 30, 2025 20:21
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.

6 participants