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

Icons using emoji #1356

Merged
merged 6 commits into from
May 10, 2022
Merged

Icons using emoji #1356

merged 6 commits into from
May 10, 2022

Conversation

dkabus
Copy link
Contributor

@dkabus dkabus commented May 5, 2022

I have implemented icons using emoji such that it works in my version of the suckless terminal st. This addresses the feature request #1346.

@dkabus dkabus changed the title Emoji Icons using emoji May 5, 2022
N-R-K
N-R-K previously requested changes May 5, 2022
Copy link
Collaborator

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

Glancing over the patch, here's a couple issues that stands out.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
src/icons.h Show resolved Hide resolved
@N-R-K N-R-K linked an issue May 5, 2022 that may be closed by this pull request
@N-R-K
Copy link
Collaborator

N-R-K commented May 7, 2022

The cell width issue should be fixed by 936d6fb, but still not entirely sure if mixing single and double width emojies are a good idea or not.

Looks ugly IMO.

image

@N-R-K N-R-K dismissed their stale review May 7, 2022 12:06

stale

src/icons.h Outdated Show resolved Hide resolved
src/icons.h Outdated Show resolved Hide resolved
src/icons-emoji.h Show resolved Hide resolved
Copy link
Collaborator

@N-R-K N-R-K left a comment

Choose a reason for hiding this comment

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

Other than the mixing of single and double width emojies, looks good.

@jarun
Copy link
Owner

jarun commented May 10, 2022

Looks clean to me. Let's hear about the width from the users. ;)

@jarun jarun merged commit 88f7015 into jarun:master May 10, 2022
@jarun
Copy link
Owner

jarun commented May 10, 2022

Thank you so much!
Personally I like it very much.

@jarun
Copy link
Owner

jarun commented May 10, 2022

The game and code emoji are not showing correctly on xubuntu 22.04, xfce4-terminal. Can you please have a look?

@jarun
Copy link
Owner

jarun commented May 10, 2022

@dkabus At commit af94c77 I removed all the additional emojis you added. I think I made it very clear in the defect that I wanted something minimal, not with additional bells and whistles. This is a public project, we do not want non-standard stuff. If you need those for your custom distro, please patch downstream.

@N-R-K N-R-K mentioned this pull request May 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2022
@@ -269,14 +280,19 @@ upload-local: sign static musl
curl -XPOST 'https://uploads.github.com/repos/jarun/nnn/releases/$(ID)/assets?name=$(BIN)-nerd-static-$(VERSION).x86_64.tar.gz' \
-H 'Authorization: token $(NNN_SIG_UPLOAD_TOKEN)' -H 'Content-Type: application/x-sharedlib' \
--upload-file $(BIN)-nerd-static-$(VERSION).x86_64.tar.gz
# upload emoji compiled static binary
tar -zcf $(BIN)-emoji-static-$(VERSION).x86_64.tar.gz $(BIN)-icons-static
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jarun Just noticed this while trying to test the released emoji static binary. It tar-ed the icons-in-terminal binary instead of the emoji binary.

N-R-K added a commit to N-R-K/nnn that referenced this pull request Jul 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] use emojis as a simple solution for icons
3 participants