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

Eliminate space around emoji #5474

Merged
merged 9 commits into from Nov 7, 2017

Conversation

Projects
None yet
5 participants
@nullkal
Collaborator

nullkal commented Oct 19, 2017

Example: https://edge.mstdn.jp/@nullkal/98856648358617597

Before:
image

After:
image

This PR eliminates space around emojis. We often use emojis like this, so I created this patch so that they are connected seamlessly.

@nullkal nullkal changed the title from Eliminatespace around emoji to Eliminate space around emoji Oct 19, 2017

nullkal added some commits Oct 19, 2017

@lynlynlynx

This comment has been minimized.

Show comment
Hide comment
@lynlynlynx

lynlynlynx Oct 19, 2017

Collaborator

I know its interesting to make looooong fox, but for common use of emojis, top and bottom paddings are needed.
ws000123
and padding-top for every status__content makes number of toots displayed fewer. Only for connecting emojos!!!😕 Ah, if there is only 2px for padding-top, I dont mind this.

However I think, removing left and right paddings is not bad, because a space is automatically inserted.

Collaborator

lynlynlynx commented Oct 19, 2017

I know its interesting to make looooong fox, but for common use of emojis, top and bottom paddings are needed.
ws000123
and padding-top for every status__content makes number of toots displayed fewer. Only for connecting emojos!!!😕 Ah, if there is only 2px for padding-top, I dont mind this.

However I think, removing left and right paddings is not bad, because a space is automatically inserted.

@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Oct 19, 2017

Collaborator

I think this PR is important because Slack has the same behavior (There is no space around custom emojis) and people importing custom emojis from Slack would be disappointed to look space around emojis they added.

I made a change not to modify unicode emoji's behavior. Because of that, this PR affects only custom emojis now. We can add spaces by editing image for custom emojis, so I think there is little concern about custom emojis.

image

And in regard to padding-top, how about decreasing status__action-bar's margin-top from 10px to 5px. This modification sets off the increase of the height (I already applied this modification in the above screenshot).

Collaborator

nullkal commented Oct 19, 2017

I think this PR is important because Slack has the same behavior (There is no space around custom emojis) and people importing custom emojis from Slack would be disappointed to look space around emojis they added.

I made a change not to modify unicode emoji's behavior. Because of that, this PR affects only custom emojis now. We can add spaces by editing image for custom emojis, so I think there is little concern about custom emojis.

image

And in regard to padding-top, how about decreasing status__action-bar's margin-top from 10px to 5px. This modification sets off the increase of the height (I already applied this modification in the above screenshot).

@lynlynlynx

This comment has been minimized.

Show comment
Hide comment
@lynlynlynx

lynlynlynx Oct 19, 2017

Collaborator

From a viewpoint of design, I cannot tolerate uneven styles (sizes, margins) between custom emojis and normal emojis (Fixed, thanks!). I cannot tolerate making general design worse ONLY for connecting emojos.
General administrators don't add connenting emojos, using/adding full-size square emojis is even more general. We frequently use general emojis in every toots, but how about connecting emojos? even if you are emojo lovers, you use them at most once a week or so, huh? Users can adjust horizontal gaps by themselves, but they cannot adjust vertical gaps.

It's my opinion, I’d like to ask other opinions.


And in regard to padding-top, how about decreasing status__action-bar's margin-top from 10px to 5px. This modification sets off the increase of the height.

It's not bad, thanks.

Collaborator

lynlynlynx commented Oct 19, 2017

From a viewpoint of design, I cannot tolerate uneven styles (sizes, margins) between custom emojis and normal emojis (Fixed, thanks!). I cannot tolerate making general design worse ONLY for connecting emojos.
General administrators don't add connenting emojos, using/adding full-size square emojis is even more general. We frequently use general emojis in every toots, but how about connecting emojos? even if you are emojo lovers, you use them at most once a week or so, huh? Users can adjust horizontal gaps by themselves, but they cannot adjust vertical gaps.

It's my opinion, I’d like to ask other opinions.


And in regard to padding-top, how about decreasing status__action-bar's margin-top from 10px to 5px. This modification sets off the increase of the height.

It's not bad, thanks.

@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Oct 19, 2017

Collaborator

Other apps' behaviors

Slack

image

I set the Emoji Style to Twitter Style (Twemoji) for taking this screnshot.

Discord

image

Anyway, we need more people's opinions about this, because talk about 'General admins' without actually listen to them is meaningless.

Collaborator

nullkal commented Oct 19, 2017

Other apps' behaviors

Slack

image

I set the Emoji Style to Twitter Style (Twemoji) for taking this screnshot.

Discord

image

Anyway, we need more people's opinions about this, because talk about 'General admins' without actually listen to them is meaningless.

nullkal added some commits Oct 19, 2017

@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Oct 19, 2017

Collaborator

When I took a screenshot of Slack I noticed that Slack has no space around not only custom emojis but also normal emojis. I think this behavior is better now, and reverted 6a5bdf0 and 54a8c60.

Collaborator

nullkal commented Oct 19, 2017

When I took a screenshot of Slack I noticed that Slack has no space around not only custom emojis but also normal emojis. I think this behavior is better now, and reverted 6a5bdf0 and 54a8c60.

@noraworld

This comment has been minimized.

Show comment
Hide comment
@noraworld

noraworld Oct 25, 2017

Contributor

I like this because some users make a big emoji by concatenating them like looooong fox. Another approach I think is better is to emojify with no space, not to change the appearance. For example, Fox:fox::fox: is converted to Fox🦊🦊.

This is also convenient for those who don't prefer a space between a word and an emoji. Japanese has no space unlike English, so I often glue a word and an emoji together, e.g. かえる🐸, no space between them.

To emojify with no space is not easy because a shortcode is closed with two colons. In other words we must discriminate between first colon and last colon. But I have implemented it before in other project. I'll make a pull request if this approach is favored a lot and no one doesn't do.

I'd also like to ask your opinions.

Contributor

noraworld commented Oct 25, 2017

I like this because some users make a big emoji by concatenating them like looooong fox. Another approach I think is better is to emojify with no space, not to change the appearance. For example, Fox:fox::fox: is converted to Fox🦊🦊.

This is also convenient for those who don't prefer a space between a word and an emoji. Japanese has no space unlike English, so I often glue a word and an emoji together, e.g. かえる🐸, no space between them.

To emojify with no space is not easy because a shortcode is closed with two colons. In other words we must discriminate between first colon and last colon. But I have implemented it before in other project. I'll make a pull request if this approach is favored a lot and no one doesn't do.

I'd also like to ask your opinions.

@Gargron

Gargron approved these changes Nov 7, 2017

@Gargron Gargron merged commit 3f16caa into tootsuite:master Nov 7, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

yi0713 added a commit to yi0713/mastodon that referenced this pull request Nov 8, 2017

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Dec 5, 2017

Member

This PR introduced 5px extra space between status text and username which looks very odd. It really stands out when you switch between 2.0.0 and master frontends. Can that detail be fixed @nullkal @lynlynlynx ?

Member

Gargron commented Dec 5, 2017

This PR introduced 5px extra space between status text and username which looks very odd. It really stands out when you switch between 2.0.0 and master frontends. Can that detail be fixed @nullkal @lynlynlynx ?

@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Dec 5, 2017

Collaborator

I added the space because emojis is cropped without it. Another solution is overflow: visible.

Collaborator

nullkal commented Dec 5, 2017

I added the space because emojis is cropped without it. Another solution is overflow: visible.

@lynlynlynx

This comment has been minimized.

Show comment
Hide comment
@lynlynlynx

lynlynlynx Dec 5, 2017

Collaborator

This PR makes emojis as large as they overflow height of letters, so it is impossible to keep exactly the same as before.
I have two suggestions below, please give me your opinion @Gargron .

master suggestion 1
2px extra space
suggestion 2
0px extra space and move emojis 2px lower
ws000127 ws000128 ws000129

This PR reduces the margin between status text and action buttons from 10px to 5px. Should it be also restored? (I think 5px is enough, ummm)

Collaborator

lynlynlynx commented Dec 5, 2017

This PR makes emojis as large as they overflow height of letters, so it is impossible to keep exactly the same as before.
I have two suggestions below, please give me your opinion @Gargron .

master suggestion 1
2px extra space
suggestion 2
0px extra space and move emojis 2px lower
ws000127 ws000128 ws000129

This PR reduces the margin between status text and action buttons from 10px to 5px. Should it be also restored? (I think 5px is enough, ummm)

@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Dec 5, 2017

Collaborator

Suggestion: add overflow: visible to .status__content, .reply-indicator__content:

2017-12-05 17 46 37

Is there any reasons to set overflow: hidden to it?

Collaborator

nullkal commented Dec 5, 2017

Suggestion: add overflow: visible to .status__content, .reply-indicator__content:

2017-12-05 17 46 37

Is there any reasons to set overflow: hidden to it?

@lynlynlynx

This comment has been minimized.

Show comment
Hide comment
@lynlynlynx

lynlynlynx Dec 5, 2017

Collaborator

overflow: visible revokes the margin between status text and username, it's not good.

ws000008

Collaborator

lynlynlynx commented Dec 5, 2017

overflow: visible revokes the margin between status text and username, it's not good.

ws000008

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Dec 5, 2017

Member

@lynlynlynx Both your suggestions (1, 2) look better than master to me. I think 2 is best. And yes I want the margin restored too...

Member

Gargron commented Dec 5, 2017

@lynlynlynx Both your suggestions (1, 2) look better than master to me. I think 2 is best. And yes I want the margin restored too...

@Gargron Gargron referenced this pull request Dec 5, 2017

Merged

Bump version to 2.1.0rc1 #5834

10 of 10 tasks complete

cobodo pushed a commit to cobodo/mastodon that referenced this pull request Dec 6, 2017

Eliminate space around emoji (tootsuite#5474)
* Eliminate space around emoji

* More improve emoji style

* Make more compatible with Twemoji

* Make scss-lint happy

* Make not modify normal emoji's behavior

* Decrease status__action-bar's margin-top to 5px

* Make the test be passed

* Revert "Make the test be passed"

This reverts commit 54a8c60.

* Revert "Make not modify normal emoji's behavior"

This reverts commit 6a5bdf0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment