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.

Sign up for GitHub

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

Jump to bottom

[TwigBridge] Add emojify twig filter #54432

Merged
merged 1 commit into from Apr 5, 2024
Merged

[TwigBridge] Add emojify twig filter #54432

merged 1 commit into from Apr 5, 2024

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Mar 28, 2024 •
edited

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues
License MIT

My most important PR of the year I guess 😅

image

@carsonbot carsonbot added this to the 7.1 milestone Mar 28, 2024
stof
stof requested changes Mar 28, 2024
View reviewed changes
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this have a change in TwigBundle to register the new extension when the symfony/emoji component is installed ?

src/Symfony/Bridge/Twig/Tests/Extension/EmojiExtensionTest.php Outdated Show resolved Hide resolved
@smnandre
Copy link
Contributor

Do you plan to handle locale and emoji data ? How would the method change ? Should we implement all at once ?

@smnandre
Copy link
Contributor

emojify should be added in the UndefinedCallableHandler i think

--

Will you make the change in TwigBundle in a following PR or want to release both at same time ?

@lyrixx
Copy link
Member Author

lyrixx commented Mar 29, 2024

Do you plan to handle locale and emoji data ?

No need I guess...

@lyrixx lyrixx requested a review from yceruto as a code owner March 29, 2024 14:24
@lyrixx
Copy link
Member Author

lyrixx commented Mar 29, 2024

Shouldn't this have a change in TwigBundle to register the new extension when the symfony/emoji component is installed ?

Done 👍🏼

@lyrixx
Copy link
Member Author

lyrixx commented Mar 29, 2024

emojify should be added in the UndefinedCallableHandler i think

Good idea. 👍🏼 Thanks

@lyrixx
Copy link
Member Author

lyrixx commented Mar 29, 2024 •
edited

Do you plan to handle locale and emoji data ? How would the method change ? Should we implement all at once ?

I now allows all catalog, and let the Emoji transliterator do all the checks.

@smnandre
Copy link
Contributor

Maybe a minor doubt about the name... as the slug filter is just "slug" and not "slugify" ... maybe "|emoji" ?

Nice "most important PR of the year" ;)

@lyrixx lyrixx changed the title [TwigBridge] Add emojify twig filter [TwigBridge] Add emoji twig filter Apr 1, 2024
yceruto
yceruto approved these changes Apr 1, 2024
View reviewed changes
Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks!

(I left an optional request; feel free to ignore it if you think it's clear enough as it is)

src/Symfony/Bridge/Twig/Extension/EmojiExtension.php Outdated Show resolved Hide resolved
@javiereguiluz javiereguiluz added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Apr 2, 2024
nicolas-grekas
nicolas-grekas reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was fine with "emojify" personnaly, looks more like a verb than just "emoji" ("slug" is already a verb so "slugify" is not needed? does that work?)

@lyrixx
Copy link
Member Author

lyrixx commented Apr 5, 2024 •
edited

Pr rebased, and updated

@lyrixx lyrixx changed the title [TwigBridge] Add emoji twig filter [TwigBridge] Add emojify twig filter Apr 5, 2024
nicolas-grekas
nicolas-grekas approved these changes Apr 5, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most important PR of 7.1! 😁

@fabpot
Copy link
Member

fabpot commented Apr 5, 2024

Thank you @lyrixx.

@fabpot fabpot merged commit cd0dbb8 into symfony:7.1 Apr 5, 2024
6 of 10 checks passed
@fabpot fabpot mentioned this pull request Apr 5, 2024
Open
@lyrixx lyrixx deleted the twig-emoji branch April 5, 2024 13:14
@Kocal Kocal mentioned this pull request Apr 5, 2024
Open
@xabbuh xabbuh mentioned this pull request Apr 9, 2024
Merged
fabpot added a commit that referenced this pull request Apr 10, 2024
@fabpot
minor #54540 [TwigBundle] re-allow Twig bridge 6.4 and 7.0 (xabbuh)
9b323c6 
This PR was merged into the 7.1 branch.

Discussion
----------

[TwigBundle] re-allow Twig bridge 6.4 and 7.0

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

The change was not necessary in #54432.

Commits
-------

40d2155 re-allow Twig bridge 6.4 and 7.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@stof stof stof requested changes

@smnandre smnandre smnandre left review comments

@derrabus derrabus derrabus left review comments

@alamirault alamirault alamirault left review comments

@fabpot fabpot fabpot approved these changes

@welcoMattic welcoMattic welcoMattic approved these changes

@yceruto yceruto yceruto approved these changes

Assignees
No one assigned
Labels
Feature ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed TwigBridge
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

None yet