-
Notifications
You must be signed in to change notification settings - Fork 48
feat: Add support for feed subscription #94
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
Conversation
Signed-off-by: Sahil Kumar <xdsahil@gmail.com>
Signed-off-by: Sahil Kumar <xdsahil@gmail.com>
Signed-off-by: Sahil Kumar <xdsahil@gmail.com>
Signed-off-by: Sahil Kumar <xdsahil@gmail.com>
Signed-off-by: Sahil Kumar <xdsahil@gmail.com>
|
@ferhatelmas I have a different formatting configuration in my IntelliJ, so can you also maybe reformat all and push. |
@xsahil03x don't worry about it, will update CI to do it automatically. |
Signed-off-by: xsahil03x <xdsahil@gmail.com>
Signed-off-by: xsahil03x <xdsahil@gmail.com>
|
@peterdeme let's review and try to release this |
| if (url.startsWith("http")) { | ||
| url = url.replace("http", "ws"); | ||
| } else if (url.startsWith("https")) { | ||
| url = url.replace("https", "wss"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this logic will work how you imagined. You need to swap the conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xsahil03x looks good to me except for my small comment. Can you add tests too?
|
@xsahil03x @ferhatelmas I copied the contents of this PR and opened a new one: #110 Please follow that one. @xsahil03x - thanks so much for your contribution 🙏 we'll try to release it ASAP. |
Fixes: #55, Porting our Dart implementation in Java
https://github.com/GetStream/stream-feed-flutter
A sample usage