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

Fix emoji replacements, make emoji images consistent #12567

Merged
merged 4 commits into from
Aug 24, 2020

Conversation

silverwind
Copy link
Member

  • Fix emoji not being replaced in issue title change text
  • Make the image attributes consistent, add alt, remove align

Before:
Screen Shot 2020-08-22 at 20 02 15

After:
Screen Shot 2020-08-22 at 21 01 15

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 22, 2020
@lafriks lafriks added the topic/ui Change the appearance of the Gitea UI label Aug 22, 2020
@lafriks
Copy link
Member

lafriks commented Aug 22, 2020

Tests seem to fail

@lafriks lafriks added this to the 1.13.0 milestone Aug 22, 2020
@silverwind
Copy link
Member Author

Fixed

@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 Aug 22, 2020
@silverwind
Copy link
Member Author

Hmm there seems to be some double escaping going on which makes pull_create_test.go fail, will check out later.

- Fix emoji not being replaced in issue title change text
- Make the image attributes consistent, add alt, remove align
@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 Aug 23, 2020
@CL-Jeremy
Copy link
Contributor

CL-Jeremy commented Aug 23, 2020

This could lead to some confusion when changing title between emoji literals and shorthands. Not a big thing though.

Anyway, found this amazing snippet to fix :gitea: not getting struck through, as well as the misalignment due to span.emoji having a vertical-align offset:

strike {
    text-decoration: none;
    position: relative;
    display: inline-block;
}
strike:after {
    content: "";
    position: absolute;
    bottom: 0;
    left: 0;
    height: calc(50% - 0.5px);
    width: 100%;
    border-top: 1px solid;
}

Bildschirmfoto 2020-08-23 um 20 47 59

Edit: changing from strike/strike:after to strike .emoji/strike .emoji:after fixes the wrapping issue, but again they seem to misalign a bit.

Changing vertical-align of .emoji from -.075em to -.05em and that of .emoji img from -.15em to -.175em fixed the alignment issue, but there always seems to be an 0.025em offset between the image ‘emojis’ and real emojis on my macOS test system. That is, if I don't change vertical-align, but change strike:after { height } from calc(50% - .5px) to 50%, :gitea: would misalign and real emojis would look fine, while currently emojis misaligns and :gitea: looks okay.

@silverwind
Copy link
Member Author

silverwind commented Aug 23, 2020

Not sure if it's a good idea to alter browser styles like that (some browser might want render strikethrough lines differently), but I'd do a slight variation (perfect centering and line does not interfere with clicks):

strike {
  text-decoration: none;
  position: relative;
}

strike::after {
  content: "";
  position: absolute;
  left: 0;
  right: 0;
  top: calc(50% - 0.5px);
  height: 1px;
  background: currentcolor;
  pointer-events: none;
}

Probably stuff for another PR.

@silverwind
Copy link
Member Author

silverwind commented Aug 23, 2020

Also, that method would break on wrapped lines, so I don't think it's a good idea.

image

@CL-Jeremy
Copy link
Contributor

Also, that method would break on wrapped lines

See my updated comment. There are some tests to be done there.

@silverwind
Copy link
Member Author

You can probably never perfectly immidate browser strikethrough styles because the spec does not dictate them to be pixel-perfect so this will always be kind of a hack.

@silverwind
Copy link
Member Author

FWIW, one can also not strikethrough GitHub's images: . ❤️ :shipit: ❤️

@CL-Jeremy
Copy link
Contributor

CL-Jeremy commented Aug 23, 2020

Confirmed to be Chromium-specific. Firefox looks perfect (my suggested imitation resembles the graphical rendering of Chromium though, not Firefox).

  top: calc(50% - 0.5px);

In your variation it should be calc(50% + 0.5px)

Edit: by Chromium-specific I mean the fact Firefox strikes through the entire <strike></strike> as if it were a span, while Chromium seems to apply text-decoration: line-through to each level inside the tag, creating the visual glitch. The glitch also changes as text gets resampled while zooming in and out. There isn't a good solution to that for now.

@techknowlogick
Copy link
Member

🚀

@techknowlogick techknowlogick merged commit ee04731 into go-gitea:master Aug 24, 2020
@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. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants