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

Add username autocompletion popup menu #2866

Merged
merged 10 commits into from
Jun 19, 2021

Conversation

talneoran
Copy link
Contributor

@talneoran talneoran commented Jun 6, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR adds a popup menu for autocompleting usernames with an '@' prefix, similar to the existing one for emotes with ':'.

  • Showing the popup is an optional feature with a toggle setting turned on by default, when the setting is off you can still autocomplete as usual.
  • The strategy for finding usernames to complete should be the same as for the existing @ autocompletion.

When testing, also test the emote autocompletion menu, as this addition can affect it, and the implementation is also inspired by it.
Also looking for feedback on the appearance of the menu.

Closes #1979

@zneix zneix self-requested a review June 6, 2021 17:25
@anqueue
Copy link
Contributor

anqueue commented Jun 6, 2021

Emote and username autocompletion menus work as intended for me.

One thing about the appearance. I think it would be nice to have some sort of indicator like DankChat does to indicate that it is a person. I provided some examples below:

DankChat:
DankChat

Current PR:
Current PR

Requested Changes:
Requested Changes

Also the username doesn't seem to be vertically centered.

@Mm2PL
Copy link
Collaborator

Mm2PL commented Jun 6, 2021

I dislike the code duplication. I would probably have reused the existing :-popup code and allowed it to show text without images. I haven't tested the functionality yet.

@talneoran
Copy link
Contributor Author

talneoran commented Jun 6, 2021

One thing about the appearance. I think it would be nice to have some sort of indicator like DankChat does to indicate that it is a person.

Not sure about this. The web chat equivalent contains only text, and unlike the DankChat example you provided, here it is clear when you are auto completing an emote vs a username based on the prefix used. Open to more opinions about this though.

Also the username doesn't seem to be vertically centered.

I think this is also the case in the emote completion popup, just less noticeable (Edit: maybe not). I will look into it though.

I dislike the code duplication. I would probably have reused the existing :-popup code and allowed it to show text without images.

I mostly agree, it just wasn't instantly obvious what the best way to combine this with the existing emote menu is, and I wanted to get something that works quickly. I can definitely try to do some more combining of the functionality and improve the code. If you have any more specific ideas I'd like to hear them.

@talneoran
Copy link
Contributor Author

I combined the code for the emote completion menu and the new username menu, now definitely test the emote completion as well when testing as it is handled by the same code.
Also usernames should be vertically centered now.

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me
I don't think we need to differentiate between users and emotes the same way DankChat does considering these two popups are activated in different ways (@ for user completion and : for emote completion).

@pajlada pajlada enabled auto-merge (squash) June 19, 2021 13:36
@pajlada pajlada merged commit f605221 into Chatterino:master Jun 19, 2021
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jun 19, 2021
Now we're on commit d6b5921; Changes from upstream we pulled:

- Major: Added username autocompletion popup menu when typing usernames with an @ prefix. (Chatterino#1979, Chatterino#2866)
- Minor: The /live split now shows channels going offline. (Chatterino#2880)
- Minor: Now shows deletions of messages like timeouts (Chatterino#1155, Chatterino#2841, Chatterino#2867, Chatterino#2874)
- Bugfix: Moderation buttons now show the correct time unit when using units other than seconds. (Chatterino#1719, Chatterino#2864)
- Bugfix: Fixed bit and new subscriber emotes not (re)loading in some rare cases. (Chatterino#2856, Chatterino#2857)
@talneoran talneoran deleted the feat/username-completion-popup branch June 27, 2021 12:06
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.

Suggest viewers when user types @name
4 participants