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

[ECO-4234] Updates Push Example to include Channels #1879

Merged
merged 6 commits into from
Mar 13, 2024
Merged

Conversation

umair-ably
Copy link
Contributor

Updated.Push.Example.mov
  • Users can now subscribe to 2 predefined example channels to test Channels based push.
  • Updated the UX to be more in line with Ably's Design System.

@umair-ably umair-ably self-assigned this Feb 22, 2024
@github-actions github-actions bot temporarily deployed to staging/pull/1879/features February 22, 2024 20:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1879/jazzydoc February 22, 2024 20:59 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1879/features March 4, 2024 21:56 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1879/jazzydoc March 4, 2024 22:01 Inactive
@maratal maratal linked an issue Mar 5, 2024 that may be closed by this pull request
@maratal
Copy link
Collaborator

maratal commented Mar 7, 2024

Looks really cool, thanks @umair-ably
A couple of suggestions:

  • "Subscribe" buttons might want to have more friendly captions.
  • "Close" icon on a subscription button should be in a different state (for unsubscribe).
  • After subscription fails it should show an error. I think it's better to introduce a dedicated control for this (a TextView in the bottom for example), where all the diagnostics info is displayed. Alerts are kinda headache in the SwiftUI (you can do this in a different PR later).

@maratal
Copy link
Collaborator

maratal commented Mar 11, 2024

I think sending by clientId also should be added to this example. It's the same code, but you set clientId instead of deviceId in the recipient dictionary.

@github-actions github-actions bot temporarily deployed to staging/pull/1879/features March 12, 2024 13:50 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1879/jazzydoc March 12, 2024 13:55 Inactive
@umair-ably
Copy link
Contributor Author

I think sending by clientId also should be added to this example. It's the same code, but you set clientId instead of deviceId in the recipient dictionary.

The readme already covers this use-case using the Push Inspector. When running on a single device/simulator, push via client or device behave the same - you only really notice the difference if you had several simulators running. Could be something worth adding as a future improvement, but not a pressing improvement for the key things this example showcases imo

@umair-ably
Copy link
Contributor Author

Looks really cool, thanks @umair-ably A couple of suggestions:

  • "Subscribe" buttons might want to have more friendly captions.
  • "Close" icon on a subscription button should be in a different state (for unsubscribe).
  • After subscription fails it should show an error. I think it's better to introduce a dedicated control for this (a TextView in the bottom for example), where all the diagnostics info is displayed. Alerts are kinda headache in the SwiftUI (you can do this in a different PR later).
  • I left the Subscribe captions more "code-based" as that is the names the readme walks the user through when setting up the channel capabilities in the dashboard
  • Good spot! I've updated this.
  • Agreed, error handling in general is pretty naff in this. Will look to update in a future PR.

@umair-ably umair-ably requested a review from maratal March 13, 2024 03:44
@jamienewcomb
Copy link
Member

Hey team , unless there is anything generally wrong this is a vast improvement on what we had before so lets work on this idea and get it landed.

any further enhancements can be implemented as required

cc @lawrence-forooghian @umair-ably @maratal

@maratal
Copy link
Collaborator

maratal commented Mar 13, 2024

I've created an issue for the error handling #1888

Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

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

LGTM

@umair-ably umair-ably merged commit a237cea into main Mar 13, 2024
7 checks passed
@umair-ably umair-ably deleted the ECO-4234 branch March 13, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

AblyPushExample should be able to send/receive pushes on the channel
3 participants