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

Allow labels to contain emoji #6063

Merged
merged 7 commits into from
Feb 16, 2019
Merged

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Feb 13, 2019

Resolves #6062

In my opinion they seem a little bulky...
Look at below comments for updated images.
issue list emoji
issue view emoji

Minor cleanup of tribute code in footer.tmpl

Signed-off-by: jolheiser <john.olheiser@gmail.com>
Signed-off-by: jolheiser <john.olheiser@gmail.com>
@codecov-io
Copy link

codecov-io commented Feb 13, 2019

Codecov Report

Merging #6063 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6063      +/-   ##
==========================================
+ Coverage   38.86%   38.87%   +<.01%     
==========================================
  Files         345      345              
  Lines       49507    49508       +1     
==========================================
+ Hits        19241    19246       +5     
+ Misses      27483    27480       -3     
+ Partials     2783     2782       -1
Impacted Files Coverage Δ
routers/repo/issue_label.go 46.76% <0%> (-0.34%) ⬇️
models/repo_list.go 64.55% <0%> (+1.26%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c8ac1a...0a59606. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 13, 2019
@techknowlogick techknowlogick added pr/wip This PR is not ready for review type/enhancement An improvement of existing functionality labels Feb 13, 2019
@techknowlogick techknowlogick added this to the 1.9.0 milestone Feb 13, 2019
@MarkusAmshove
Copy link
Contributor

I did the same find and replace to test this out locally, but you've beat me in creating a PR :-)

I've seen that in the labels overview the labels to have a label icon at the beginning, maybe this has a different css class and fits better?

@jolheiser
Copy link
Member Author

Looks as though Semantic got excited about label images and made their height !important
semantic

As much as I hate using !important to fight other !important rules, it seems that until we re-design without Semantic this might be the best choice. I will add another style for these labels for now to correct the emoji size.

Compare these new images with the old ones in the original comment.
issue list emoji 2
issue view emoji 2

@jolheiser jolheiser changed the title WIP: Allow labels to contain emoji Allow labels to contain emoji Feb 14, 2019
@silverwind
Copy link
Member

silverwind commented Feb 14, 2019

Semantic is full of weird hacks like that 😢

Looking at your screenshots, I think the emojis are still a little bit too big and seem not exactly vertically centered in some cases. Maybe you could get them a bit closer to GitHub's styling:

g-emoji {
    font-family: Apple Color Emoji,Segoe UI,Segoe UI Emoji,Segoe UI Symbol;
    font-size: 1.2em;
    font-weight: 400;
    line-height: 20px;
    vertical-align: middle;
}

g-emoji img {
    height: 1em;
    width: 1em;
}

.IssueLabel .g-emoji {
    display: inline-block;
    font-size: 1em;
    line-height: 1;
    position: relative;
    top: -.05em;
}

.IssueLabel--big .g-emoji {
    display: inline-block;
    margin-top: -1px;
}

@jolheiser
Copy link
Member Author

jolheiser commented Feb 14, 2019

Hmmm....I'm not sure I agree. This is at 1em and I think it's a tad too small, some of the emoji are getting hard to recognize.
Ultimately I'll go with what most people prefer, but as it stands I think I prefer the images above this one.
The images above are at 1.5em for reference, so there's technically .5em lee-way to work with. 😉
issue view emoji 3

@silverwind
Copy link
Member

silverwind commented Feb 15, 2019

I like them at 1em, but wouldn't mind a bit bigger, maybe 1.2em. Maybe also match the size of emojis in regular text (I imagine anything bigger than 1em will introduce unwanted whitespace in the line).

…label_emoji

Update label emoji size to 1.2em
@jolheiser
Copy link
Member Author

I think 1.2em looks fine.
issue view emoji 4

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 15, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 15, 2019
@techknowlogick techknowlogick removed the pr/wip This PR is not ready for review label Feb 16, 2019
Makefile Outdated Show resolved Hide resolved
@lafriks lafriks merged commit 0b72c00 into go-gitea:master Feb 16, 2019
@lafriks lafriks modified the milestones: 1.9.0, 1.8.0 Feb 16, 2019
@lafriks
Copy link
Member

lafriks commented Feb 16, 2019

Ouch... It was for 1.9.0... sry, already merged :)

@jolheiser
Copy link
Member Author

Ugh, sorry about the Makefile. I've been trying to keep up with that and removing it before committing.
Darn Windows being difficult.

@jolheiser jolheiser deleted the 6062_label_emoji branch February 16, 2019 18:31
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow labels to have emojis
10 participants