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

Change Twitter character limit #39109

Merged
merged 4 commits into from Nov 27, 2017

Conversation

Projects
None yet
5 participants
@ethernetcable
Contributor

ethernetcable commented Nov 24, 2017

Tweets can now be up to 280 characters, so this should be accounted for in the feedback section.

fixes #35214

@kandros

This comment has been minimized.

Show comment
Hide comment
@kandros

kandros commented Nov 25, 2017

LGTM

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 25, 2017

Member

@ethernetcable unless I am mistaken, the same old limit applies for people typing in CJK languages, no? So your fix does not seem to account for that.

Member

bpasero commented Nov 25, 2017

@ethernetcable unless I am mistaken, the same old limit applies for people typing in CJK languages, no? So your fix does not seem to account for that.

@bpasero bpasero self-assigned this Nov 25, 2017

@ethernetcable

This comment has been minimized.

Show comment
Hide comment
@ethernetcable

ethernetcable Nov 25, 2017

Contributor

@bpasero you are right, I have changed the limit if the user's display language is not CJK.

Contributor

ethernetcable commented Nov 25, 2017

@bpasero you are right, I have changed the limit if the user's display language is not CJK.

ethernetcable added some commits Nov 25, 2017

@bpasero bpasero added this to the On Deck milestone Nov 25, 2017

@ethernetcable

This comment has been minimized.

Show comment
Hide comment
@ethernetcable

ethernetcable Nov 26, 2017

Contributor

Can anyone tell me why this would fail hygiene? I can't work it out.

Contributor

ethernetcable commented Nov 26, 2017

Can anyone tell me why this would fail hygiene? I can't work it out.

@bpasero bpasero merged commit 6aae8ac into Microsoft:master Nov 27, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 27, 2017

Member

Thanks 👍

Member

bpasero commented Nov 27, 2017

Thanks 👍

@chrbala

This comment has been minimized.

Show comment
Hide comment
@chrbala

chrbala Nov 27, 2017

This doesn't look right to me. The limit is calculated in a per-character basis, so it won't show the correct limit for mixed characters or emoji (also double-counted)

chrbala commented Nov 27, 2017

This doesn't look right to me. The limit is calculated in a per-character basis, so it won't show the correct limit for mixed characters or emoji (also double-counted)

@kumarharsh

This comment has been minimized.

Show comment
Hide comment
@kumarharsh

kumarharsh Nov 27, 2017

Contributor

@bpasero this won't work for the reasons I highlighted in this comment. Twitter will update their library likely in mid-Dec to give a correct solution.

Contributor

kumarharsh commented Nov 27, 2017

@bpasero this won't work for the reasons I highlighted in this comment. Twitter will update their library likely in mid-Dec to give a correct solution.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 27, 2017

Member

@chrbala @kumarharsh I would still argue this is an improvement to the way it was before. We never accounted for mixed characters or emojis in our previous solution. We can still tweak this further to resolve those issues, but for the majority of people the limit of 280 will be correct.

Member

bpasero commented Nov 27, 2017

@chrbala @kumarharsh I would still argue this is an improvement to the way it was before. We never accounted for mixed characters or emojis in our previous solution. We can still tweak this further to resolve those issues, but for the majority of people the limit of 280 will be correct.

@kumarharsh

This comment has been minimized.

Show comment
Hide comment
@kumarharsh

kumarharsh Nov 27, 2017

Contributor

Yeah, makes sense. Good enough is better than nothing 👍

Contributor

kumarharsh commented Nov 27, 2017

Yeah, makes sense. Good enough is better than nothing 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment