-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: Update social links #173
fix: Update social links #173
Conversation
I am not sure if any of these changes are the right approach or if this conflicts with any other changes in progress. But this change fixes the low resolution and dark mode icons for the linked PR that adds an in-app about screen. |
…ut will load using curl and Android Chrome)
Waiting on the clarification requested here: ReVanced/revanced-patches#2981 (comment) |
For now the icons are removed. This PR is still has some changes, notable:
|
The API does not return any semantics regarding ordering. If we want to introduce a controlled way of ordering, that should be done either by documenting it, or adding a an priority field for example with an integer value. |
As I don't touch any Android development and no one ever asked for this, it was never implemented. I will look into it during the weekend.
Why would ordering be relevant here? Different clients can have different ordering requirements and that step can be better executed in the client side I think. |
The ordering would be fixed for every client. But again, I said, if we want that, instead of changing the order here just to produce ordering as a side effect on the clients |
Currently the only ordering a client can pick, is if it wants to sort the API json response alphabetically or not. Alphabetical is not the desired ordering used here, so the client has no way to make the ordering changes. Changing the ordering for all clients is ok, then they'll also use the same more consistent presentation. |
Co-authored-by: oSumAtrIX <[email protected]>
Is it possible to document in the API that ordering is implicated? |
Can add some comments to the API. But I have no idea where documentation goes here. This language and deployment is foreign to me. |
Since this PR was cut down and no longer has icon links, the PR is no longer a must have change. The social links are being refactored in https://www.github.com/ReVanced/revanced-api/pull/142 making this PR redundant and not needed. |
Changes relevant to https://www.github.com/ReVanced/revanced-patches/pull/2981
Of note, the setup instructions on this repo could be improved to mention how to enable https support (which is required for debugging Android using the api). I spent an hour+ trying other Sanic setup instructions, but none worked for me.
Before this PR:
Changes made with this PR: