Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] fix emojis messing up the line width#40524
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedMar 19, 2021
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
chalasr commentedMar 20, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Do we really need a new method? Can't we update |
a08f6cd toda6fcf5CompareMarionLeHerisson commentedMar 22, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hello@chalasr, thanks for the feedback. Since the method is used in 13 other places in the code, I was afraid it would break something. But after trying it and running the tests, it seems it doesn't 👍 |
Uh oh!
There was an error while loading.Please reload this page.
add tests + removed irrelevant method
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
(I applied my own review comment and fixed a few more things found meanwhile)
nicolas-grekas commentedMar 23, 2021
Thank you@MarionLeHerisson. |
Nyholm commentedApr 3, 2021
I like this feature, however, it caused some regressions. I will revert this an add it as a new feature in 5.3 instead. See#40697 |
symfony/symfony#40524will be investigated and implemented in future Termage releases
Uh oh!
There was an error while loading.Please reload this page.
Description
The emojis, because they take as much space as two characters, would cause the console to display too many spaces to complete a line, which made it uneven, as described in the issue.
The fix uses the
widthfunction instead ofstrlen. To answer@ogizanagi's comment, yes it does work with "composed" emojis.Before :
After :
Other changes
Removed two unused lines of code, the value of
$messageLineLengthwas never used.Note
I'd like to add some tests, but I don't know how since I think this depends on console client width ?
Thanks for your reviews 🙏