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

Simple Demo #140

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

Simple Demo #140

wants to merge 3 commits into from

Conversation

lolloz98
Copy link

I found the provided demo a bit difficult to read.
I thought it could be a good idea to provide an easier demo app: the developers will be able to change the server settings and to really try Scarlet, before diving into the library in their own projects.
I hope it could be of some help.

@CLAassistant
Copy link

CLAassistant commented Apr 24, 2020

CLA assistant check
All committers have signed the CLA.

@aaronweihe
Copy link
Collaborator

thanks for the contribution!

  • can you provide more details on why the demo app in Scarlet is hard to read? If there's something you particular find difficult to integrate, I'd love to hear so we can improve the documentation.
  • the demo app in Scarlet is using publicly available websocket APIs to showcase the integration, whereas the demo app in this PR uses a local environment, and
  • it looks like the demo app in this PR is a standalone, and doesn't adhere to naming/coding convention with the rest of the code base in Scarlet.

@lolloz98
Copy link
Author

lolloz98 commented Apr 24, 2020

I think that the main problem is its complexity.
It implements a lot of features and relies on many external libraries (e.g. javax and dagger annotations) which are not really related to Scarlet core usage.
There are not many comments guiding the developer through it and I believe that the call to the Scarlet Builder is a bit hidden even though it should be one of the focal points.
All of these things hide the beauty and simplicity of the library and they increase the learning curve for a new possible user.
Moreover, the app fails to connect to the server (I read that the problem has already been solved).

I came up with the simple_demo to try to implement only the features related to this library and commenting them at each step. Maybe it is too commented, but the app is so small that it should not be a problem.
It gives the possibility to the developer to look at a simple server-side and a full picture of the core of the library: just as in the talks on youtube and other posts (but still, an implementation clarifies all possible doubts).

I have set the dependencies like that on purpose because I found in some posts online a bit of confusion regarding this topic. If you are considering pulling this, I will gladly comment them out and change them (and the naming convention as well).

@thu2004
Copy link

thu2004 commented Oct 5, 2020

I just try to compile the origin demo without success. The readme file just showed some code snipper and there are many dependencies code missing.
Just saw this Simple Demo MR and I try it. It just works. Please merge it in. Very helpful demo.

@aaronweihe
Copy link
Collaborator

I just try to compile the origin demo without success. The readme file just showed some code snipper and there are many dependencies code missing.
Just saw this Simple Demo MR and I try it. It just works. Please merge it in. Very helpful demo.

can you open an issue with what error message you got with compiling with the demo app in the repo?

@aaronweihe
Copy link
Collaborator

@lolloz98 is there anything you think is missing in the https://github.com/Tinder/Scarlet#usage?

@aaronweihe
Copy link
Collaborator

the first step for this PR to be reviewed further is to make use of the existing infrastructure set up in the repo, e.g., centralized .gitignore and adding this as an additional module rather than creating a new Gradle project etc.

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.

5 participants