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

Highlight own username #209

Closed
wants to merge 2 commits into from

Conversation

rsammelson
Copy link
Contributor

@rsammelson rsammelson commented Jul 3, 2023

Fixes #161

@rsammelson rsammelson force-pushed the username-improvements branch from b41b938 to d4c7f1f Compare July 3, 2023 14:53
@rsammelson
Copy link
Contributor Author

@aeharding, this PR is ready I think. Can you merge it with a merge commit so that the history of the two separate commits is retained?

@rsammelson rsammelson force-pushed the username-improvements branch from d4c7f1f to c687032 Compare July 4, 2023 02:44
@rsammelson rsammelson mentioned this pull request Jul 4, 2023
@rsammelson rsammelson marked this pull request as draft July 4, 2023 17:55
@rsammelson rsammelson force-pushed the username-improvements branch from c687032 to 2f5a74d Compare July 8, 2023 05:55
@rsammelson rsammelson marked this pull request as ready for review July 8, 2023 05:56
@rsammelson
Copy link
Contributor Author

@aeharding This is ready to go again.

@rsammelson rsammelson force-pushed the username-improvements branch 4 times, most recently from df472f6 to f3a4c0a Compare July 9, 2023 20:28
@rsammelson
Copy link
Contributor Author

@aeharding What's the status on this?

@aeharding
Copy link
Owner

So a couple concerns:

  1. The db item is called instance_url_mode, yet the setting is under comments in the UI and only applies to comments. We need a good name for this that describes what this setting actually is for
  2. Should this be a global setting? What is the argument for it only applying to comments?

@rsammelson
Copy link
Contributor Author

1. The db item is called `instance_url_mode`, yet the setting is under comments in the UI and only applies to comments. We need a good name for this that describes what this setting actually is for

The idea was for the setting to possibly cover more than just comments in the future, but I can rename it to something like show_instance_url_comment.

2. Should this be a global setting? What is the argument for it only applying to comments?

Do communities not already show their instance?

@rsammelson
Copy link
Contributor Author

Actually, I already applied a change to make this apply to users in the post details, so I should move the setting to a different block.

@rsammelson rsammelson force-pushed the username-improvements branch from f3a4c0a to 17d9ca4 Compare July 9, 2023 20:58
@aeharding
Copy link
Owner

Sorry, one other question would be should it apply to community handles too? Or should that be its own setting? If its own setting, what would be the use case for that? 🤔

@aeharding
Copy link
Owner

aeharding commented Jul 9, 2023

image

Can the setting control these user handles too (this is a community page)?

@rsammelson rsammelson force-pushed the username-improvements branch from 17d9ca4 to 8d0349f Compare July 9, 2023 21:17
@rsammelson
Copy link
Contributor Author

Sorry, one other question would be should it apply to community handles too? Or should that be its own setting? If its own setting, what would be the use case for that? 🤔

I think community handles for remote communities should pretty much always show the URL, so I don't think that needs a setting.

Right now this is really an override to force showing instance URLs when they wouldn't be shown. If you want me to add the ability to never show user instance URLs, I can do that in a separate PR.

@rsammelson rsammelson force-pushed the username-improvements branch from 8d0349f to eebf7b3 Compare July 9, 2023 21:19
@rsammelson
Copy link
Contributor Author

One use case would be that if you run your own instance and find a lot of junk coming from users on a specific instance, you could remove it.

@rsammelson rsammelson force-pushed the username-improvements branch from eebf7b3 to a387b76 Compare July 9, 2023 21:30
@rsammelson
Copy link
Contributor Author

@aeharding alright I finished renaming things and added a general section, so it should hopefully make more sense now.

@rsammelson
Copy link
Contributor Author

If you want me to add the ability to never show user instance URLs, I can do that in a separate PR.

@aeharding to elaborate on this, I did some cleanup in #185 that I would prefer to have before making any more multi-select settings. Once that is merged I would like to add the ability to turn off instance URLs for people everywhere to simplify the UI for those who want that.

@rsammelson rsammelson force-pushed the username-improvements branch from a387b76 to 8adac6a Compare July 11, 2023 03:23
@aeharding
Copy link
Owner

Gotcha, I'll take a look at #185 soon.

@rsammelson
Copy link
Contributor Author

rsammelson commented Jul 12, 2023

@aeharding, that is blocked on this because they both touched the same preferences code and I didn't want to have merge conflicts, so I would prefer to add that additional feature in a follow-on PR. If I really have to I can rebase that PR onto main instead of this branch, but it will be a little painful.

@aeharding
Copy link
Owner

Oh I misunderstood, so just to confirm you'd prefer this one go first?

@rsammelson
Copy link
Contributor Author

Yes, and once this is merged I'll have two more PRs ready for you shortly.

@rsammelson rsammelson force-pushed the username-improvements branch from 8adac6a to 404476a Compare July 13, 2023 01:04
@rsammelson
Copy link
Contributor Author

@aeharding do you want any other changes here or is this good to go?

@aeharding
Copy link
Owner

I'll check it out soon!

@aeharding
Copy link
Owner

Can you remove the yellow username thing from this PR? We can discuss my concerns in the issue (I commented earlier).

Otherwise LGTM! I just tested and its great.

Maybe we should move this to a "Federation" section below post/comment sections? What do you think about that?

@aeharding
Copy link
Owner

Also I can resolve the conflicts if you'd like, just let me know.

@rsammelson rsammelson force-pushed the username-improvements branch from 404476a to 5acde9b Compare July 16, 2023 20:51
@rsammelson
Copy link
Contributor Author

@aeharding, I fixed the conflicts, but I still have the user highlight; I disabled it on the profile page like I mentioned. If you want it gone altogether let me know.

@aeharding
Copy link
Owner

I think let's pull it out of this PR. There's other concerns too, like light mode contrast, showing in inbox, consistency

@aeharding
Copy link
Owner

Also, the label for the setting is getting cut off on my iPhone. So let's shorten to "Show user instance"

@rsammelson rsammelson force-pushed the username-improvements branch 2 times, most recently from 124e324 to 6b97b4b Compare July 16, 2023 23:29
@rsammelson
Copy link
Contributor Author

showing in inbox, consistency

Can you elaborate on these two? I don't see where your own username shows up in the inbox.

@rsammelson rsammelson force-pushed the username-improvements branch from 6b97b4b to 7e9cf6f Compare July 16, 2023 23:35
@rsammelson
Copy link
Contributor Author

I also did adjust the color to improve contrast, the contrast still isn't fantastic in light mode, but it is higher than the green used for distinguished usernames.

@rsammelson rsammelson force-pushed the username-improvements branch from 7e9cf6f to b5f5816 Compare July 17, 2023 02:32
@rsammelson rsammelson changed the title Username improvements Highlight own username Jul 17, 2023
@rsammelson rsammelson force-pushed the username-improvements branch from b5f5816 to b701b86 Compare July 20, 2023 05:28
@aeharding aeharding force-pushed the main branch 2 times, most recently from 4346eb6 to 86d9319 Compare August 5, 2023 06:12
@aeharding
Copy link
Owner

I am closing this because it has become quite stale. This is something that I may consider in the future as a configurable option, but lets coordinate on the issue if interested.

@aeharding aeharding closed this Dec 5, 2023
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.

Highlight my username in comments to differentiate from other users.
2 participants