search

ReVanced / revanced-api

🚀 API server for ReVanced
https://api.revanced.app
GNU Affero General Public License v3.0
93 stars 14 forks source link

fix: Update social links #173

Closed LisoUseInAIKyrios closed 6 months ago

LisoUseInAIKyrios commented 7 months ago

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:

![Before](https://github.com/ReVanced/revanced-api/assets/118716522/33071502-6e00-42ae-a7ec-141e4d8390a5)

Changes made with this PR:

![PR Changes](https://github.com/ReVanced/revanced-api/assets/118716522/7840bdef-6460-4ce7-b57b-070c48e1c69b)
LisoUseInAIKyrios commented 7 months ago

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.

alexandreteles commented 6 months ago

Waiting on the clarification requested here: https://github.com/ReVanced/revanced-patches/pull/2981#issuecomment-2045779014

LisoUseInAIKyrios commented 6 months ago

For now the icons are removed.

This PR is still has some changes, notable:

oSumAtrIX commented 6 months ago

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.

alexandreteles commented 6 months ago

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).

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.

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.

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.

oSumAtrIX commented 6 months ago

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

LisoUseInAIKyrios commented 6 months ago

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.

oSumAtrIX commented 6 months ago

Is it possible to document in the API that ordering is implicated?

LisoUseInAIKyrios commented 6 months ago

Can add some comments to the API. Something like "Social links are pre-sorted in the preferred presentation order".

But I have no idea where documentation goes here. This language and deployment is foreign to me.

LisoUseInAIKyrios commented 6 months ago

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.