Skip to content

[Icons] Docs: Fixing links to symfony/ux-twig-component #2789

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

Merged
merged 1 commit into from
May 30, 2025

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented May 28, 2025

Q A
Bug fix? no
New feature? no
Docs? yes
Issues
License MIT

Page: https://symfony.com/bundles/ux-icons/current/index.html

@carsonbot carsonbot added Icons Status: Needs Review Needs to be reviewed labels May 28, 2025
the TwigComponent overhead is reduced by calling the IconRenderer immediately and
returning the HTML output.

.. warning::

The <twig:ux:icon> component does not support embedded content.
The `symfony/ux-twig-component`_ does not support embedded content.
Copy link
Member

Choose a reason for hiding this comment

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

? I think you're missreading this.

The twig:ux:icon component does not support embedded content.

The symfony/ux-twig-component has nothing to do with it, and I don't want to imply anything else regarding twig component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you're missreading this.

Maybe, but I'm certainly not misclicking it :-)
=> What's currently on the page is a broken link!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, now I see: In fact, there was just the second backtick missing ;-)
The word "component" was misleading me; so I changed that to "tag".

Copy link
Member

Choose a reason for hiding this comment

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

=> What's currently on the page is a broken link!

Ho i saw that, and thank you for reporting it!

But you changed the meaning on the way and this is another thing that fixing something :)

@@ -526,13 +526,13 @@ TwigComponent
~~~~~~~~~~~~~

The ``ux_icon()`` function is optimized to be as fast as possible. To deliver the
same level of performance when using the HTML syntax (``<twig:ux:icon name="..." />``),
same level of performance as if using the HTML syntax (``<twig:ux:icon name="..." />``),
Copy link
Member

Choose a reason for hiding this comment

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

No, the original meaning is the current one. The HTML syntax is the name of the syntax of TwigComponents that looks like HTML.

So here it's not "as if" but "when"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you want to say is:

The twig syntax has the same performance as the HTML syntax.

Right?

Same as in #2781 (review) - let's switch the parts:

The TwigComponent overhead is reduced to deliver the same level of performance when using the HTML syntax

=> No. Cause when I use the twig syntax, I cannot use the HTML syntax at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Not at all.

What we do say here is : using the Twig function is the fastest possible. When using the Twig Component with the HTML Syntax, we have developed tactics.

Said in another way:

"To deliver the same level of performance when using the HTML syntax... the UX Icon component [...]"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I finally got what you mean. But this is impossible to get across with the current sentence structure. I'll come up with a better suggestion.
But one question first: Is ux_icon() now faster or are they the same?

Copy link
Member

Choose a reason for hiding this comment

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

But one question first: Is ux_icon() now faster or are they the same?

If they are in fact, the same, what about removing this paragraph entirely?

Copy link
Contributor Author

@ThomasLandauer ThomasLandauer May 29, 2025

Choose a reason for hiding this comment

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

what about removing this paragraph entirely?

I didn't dare to ask that :-)

No, seriously - what about shortening this to something like:

Due to internal optimizations, there is no real speed difference between the Twig and the HTML syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Due to internal optimizations, there is no real speed difference between the Twig and the HTML syntax.

Perfect! They don't need to know how the sausage is made!

Copy link
Member

Choose a reason for hiding this comment

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

But let's keep this PR focused on just the link fix and change this in #2795.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

know how the sausage is made

A Canadian saying? :-) But you don't eat that much sausage over there, do you?

OK, I reverted it here. But that other PR is a mess already, so I won't add more stuff there...

Copy link
Member

@smnandre smnandre left a comment

Choose a reason for hiding this comment

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

Thanks for the link!

Let's revert the text changes :)

@carsonbot carsonbot added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Needs Review Needs to be reviewed Status: Needs Work Additional work is needed labels May 29, 2025
@@ -526,13 +526,13 @@ TwigComponent
~~~~~~~~~~~~~

The ``ux_icon()`` function is optimized to be as fast as possible. To deliver the
same level of performance when using the HTML syntax (``<twig:ux:icon name="..." />``),
same level of performance as if using the HTML syntax (``<twig:ux:icon name="..." />``),
Copy link
Member

Choose a reason for hiding this comment

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

Not at all.

What we do say here is : using the Twig function is the fastest possible. When using the Twig Component with the HTML Syntax, we have developed tactics.

Said in another way:

"To deliver the same level of performance when using the HTML syntax... the UX Icon component [...]"

the TwigComponent overhead is reduced by calling the IconRenderer immediately and
returning the HTML output.

.. warning::

The <twig:ux:icon> component does not support embedded content.
The ``<twig:ux:icon>`` tag does not support embedded content.
Copy link
Member

Choose a reason for hiding this comment

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

No. I'm sorry. It's not a question of tag but a question of special Twig Component

@carsonbot carsonbot added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Needs Review Needs to be reviewed Status: Needs Work Additional work is needed labels May 29, 2025
@ThomasLandauer
Copy link
Contributor Author

ThomasLandauer commented May 29, 2025

Sorry, but I'm starting to lose the overview here. I now brought back the "component"in this sentence:

The <twig:ux:icon> component does not support embedded content.

=> But in fact this entire paragraph needs to be removed, as per what you're saying at #2796 (comment)
But I can't do that right now - need to see how the page looks like after all my changes... ;-)

And reduce the text to:

Due to internal optimizations, there is no real speed difference between the Twig and the HTML syntax.

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels May 29, 2025
@smnandre
Copy link
Member

Thank you Thomas.

@smnandre smnandre merged commit 94f27c1 into symfony:2.x May 30, 2025
1 check passed
@ThomasLandauer ThomasLandauer deleted the patch-6 branch May 30, 2025 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Icons Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants