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

[FEATURE] Uniform avatar configure by AVATAR_PROXY #555

Merged
merged 6 commits into from
Sep 21, 2021
Merged

Conversation

lizheming
Copy link
Collaborator

close #525 when pr closed.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

Some questions:

  1. Is client now just need to use the link directly as avatar?
  2. Do we need to try parsing the returned link and making avatar settings in client workable? (e.g.: avatarCDN)

@lizheming
Copy link
Collaborator Author

lizheming commented Sep 20, 2021

Some questions:

  1. Is client now just need to use the link directly?
  2. Do we need to try parsing the returned link and making avatar settings in client workable? (e.g.: avatarCDN)
  1. Yes
  2. Md5 mail address will also return and all client gravatar configuration like avatarCDN and avatar works well right now. But I have updated documentation that avatarCDN, avatar and avatarForce is deprecated and will be removed in the next major version.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

Since the client is an instance now and can be updated, I will need extra function to detect if the current avatar config is the same as default config.

I will try to make this change graceful.

Edited: I got an idea to handle it.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

avatarForce can still be supported on the client, and I am not seeing such feature in server. Do we need to keep it?

Edited: So do avatar itself I think. Only proxy is set on the server, but all the related docs about client is marked as "deprecated".

@lizheming
Copy link
Collaborator Author

lizheming commented Sep 20, 2021

@Mister-Hope avatarForce and avatar also will be deprecated.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

So we are actually removing the "force fetch" feature? I understand that the type can be set in the proxy env variable, but enabling a random or time based suffix is not seen in this pr. (Or do I miss it?)

@lizheming
Copy link
Collaborator Author

@Mister-Hope All avatar configuration will move to https://github.com/waline/porter and custom it If they want. And https://github.com/waline/porter has no cache right now, so avatarForce is no needed.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

Got it, so I just need to detect if users are implict setting any avatar options, and use the old logic, otherwise just using the avatar property on comment right?

@lizheming
Copy link
Collaborator Author

@Mister-Hope Yes, you're right. I think client has comment avatar field detection feature already and do not need any more work about it, because login user comment data will export avatar field.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

But that will make all the avatar settings unavailable when server side updates, and is some kind of breaking changes. I found this out either when I am trying to finish this one.

BTW, currently is there a way for me to detect if a user is a login user?

@lizheming
Copy link
Collaborator Author

lizheming commented Sep 20, 2021

@Mister-Hope I'll update major version and add some description in the changelog.

@Mister-Hope
Copy link
Member

I am not looking forward to raise a major version, as I am planing to drop some support of old filelds in Valine when raising to V2. (Such as the Chinglish langMode)

@lizheming
Copy link
Collaborator Author

@Mister-Hope OK, so we can create v2 branch and merge those break feature into it and release v2 beta version.

@Mister-Hope
Copy link
Member

Mister-Hope commented Sep 20, 2021

Edited: BTW, currently is there a way for me to detect if a comment is posted by a login user? (Which means his avatar should be kept)

If so I can still support it without making breaking changes.

@lizheming
Copy link
Collaborator Author

lizheming commented Sep 20, 2021

@Mister-Hope a comment data has type field is posted by login user.

@lizheming lizheming merged commit ff0958e into main Sep 21, 2021
@Mister-Hope Mister-Hope deleted the feature/avatar branch October 30, 2021 16:28
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.

Avatar 配置优化
2 participants