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

fix: Update social links #173

Closed

Conversation

LisoUseInAIKyrios
Copy link
Contributor

@LisoUseInAIKyrios LisoUseInAIKyrios commented Apr 8, 2024

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

Changes made with this PR:

PR Changes

@LisoUseInAIKyrios
Copy link
Contributor Author

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.

config.py Outdated Show resolved Hide resolved
@alexandreteles alexandreteles self-assigned this Apr 9, 2024
@alexandreteles
Copy link
Contributor

Waiting on the clarification requested here: ReVanced/revanced-patches#2981 (comment)

@alexandreteles alexandreteles changed the base branch from main to dev April 9, 2024 17:59
@LisoUseInAIKyrios
Copy link
Contributor Author

For now the icons are removed.

This PR is still has some changes, notable:

  • Changed to the same links used by Manager (Discord is the most important, as the direct link opens the app and not an intermediate website)
  • Reordered the social links to use same ordering as the website

@LisoUseInAIKyrios LisoUseInAIKyrios changed the title fix: Add icon urls to social links fix: Update social links Apr 9, 2024
config.py Outdated Show resolved Hide resolved
@oSumAtrIX
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

LisoUseInAIKyrios commented Apr 10, 2024

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]>
@oSumAtrIX
Copy link
Member

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

@LisoUseInAIKyrios
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@LisoUseInAIKyrios LisoUseInAIKyrios deleted the social_thumbnails branch April 18, 2024 10:23
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.

3 participants