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 search to emote popup #3404

Merged
merged 19 commits into from
Jan 2, 2022
Merged

Add search to emote popup #3404

merged 19 commits into from
Jan 2, 2022

Conversation

acdvs
Copy link
Contributor

@acdvs acdvs commented Dec 15, 2021

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

A search bar at the top of the emote popup that filters all emotes across every tab as you type. Uses a new ChannelView to combine all emote sets and is initially hidden until something is typed.

emote_search

@acdvs acdvs marked this pull request as ready for review December 19, 2021 11:44
@zneix zneix self-requested a review December 19, 2021 11:58
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.

Nice feature addition! Thank you for the contribution

Got some small tweaks/concerns covered in the comments that I'd like adressed before merging this in. Let me know if you have any questions!

Open questions for others who test this:

  1. How should pressing Escape behave when the search input is filled? Right now it closes the popup, but I feel like maybe it should clear the search entry first if there's a search query active
  2. Should the search input be selected by default? I don't believe it is right now, but I think it would be nice if it was

src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.hpp Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.hpp Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.hpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
@acdvs acdvs requested a review from pajlada December 29, 2021 10:30
@zneix
Copy link
Collaborator

zneix commented Jan 1, 2022

Should the search input be selected by default? I don't believe it is right now, but I think it would be nice if it was

I believe it should be - we already have this as some sort of standard across different dialogs that implement the search input anyway (e.g. Settings dialog, Message Search dialog)

@pajlada
Copy link
Member

pajlada commented Jan 1, 2022

Should the search input be selected by default? I don't believe it is right now, but I think it would be nice if it was

I believe it should be - we already have this as some sort of standard across different dialogs that implement the search input anyway (e.g. Settings dialog, Message Search dialog)

Got code in my local repository fixing this, and adding support for the hotkey system.

Will push together with a merge when another PR has been merged in to save on CI hours.

src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
src/widgets/dialogs/EmotePopup.cpp Outdated Show resolved Hide resolved
@pajlada pajlada enabled auto-merge (squash) January 2, 2022 14:40
@pajlada pajlada merged commit 8e5468c into Chatterino:master Jan 2, 2022
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jan 2, 2022
Now we're on commit 14d11b6; Changes from upstream we've pulled

- Minor: Add search to emote popup. (Chatterino#3404)
- Minor: Messages can now be highlighted by subscriber or founder badges. (Chatterino#3445)
- Bugfix: Fixed crash that could occur if the user opens/closes ChannelViews (e.g. EmotePopup, or Splits) then modifies the showLastMessageIndicator setting. (Chatterino#3444)
- Dev: Batch checking live status for channels with live notifications that aren't connected. (Chatterino#3442)
- Dev: Added /fakemsg command for debugging (Chatterino#3448)
- Dev: Notebook::select\* functions now take an optional `focusPage` parameter (true by default) which keeps the default behaviour of selecting the page after it has been selected. If set to false, the page is _not_ focused after being selected. (Chatterino#3446)
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Jan 28, 2022
Now we're on commit 4e422b3; Changes from upstream we've pulled

+- Minor: Add search to emote popup. (Chatterino#3404, Chatterino#3527) // Chatterino#3527 got added, Chatterino#3404 was alredy merged in
+- Minor: Colorize the entire split header when focused. (Chatterino#3379)
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