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.

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

[Icons] Improve setup instructions #2664

Merged
merged 1 commit into from
Apr 4, 2025
Merged

[Icons] Improve setup instructions #2664

merged 1 commit into from
Apr 4, 2025

Conversation

dbu
Copy link
Contributor

@dbu dbu commented Mar 29, 2025

Q A
Bug fix? no
New feature? no
Docs? yes
Issues https://symfony-devs.slack.com/archives/C01FN4EQNLX/p1743272426739939
License MIT

i missed that i need the http-client installed, the bundle silently reduces functionality without it. i try to make it more clear what needs to be installed by mentioning the optional dependencies in the installation section, and by moving the http client hint to the "icon sets" section rather than having it in the middle of the syntax how to render icons.

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Mar 29, 2025
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 29, 2025
Comment on lines +19 to +26
If you plan to use provided icon sets, make sure that you have the HTTP client installed:

.. code-block:: terminal

$ composer require symfony/http-client
Copy link
Member

Choose a reason for hiding this comment

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

Could you use another type of block here, or even maybe just a paragraph?

I'd like to stay in line with the symfony.com documentation "standard", and only document here the strict minimum.

Too many information could give a false impression of complexity

It's a delicate art to not overwelm users with info when they do not need it yet, but also not falling in the opposite. (your issue is a proof of that!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my thinking was that the optional dependencies are easy to overlook when not reading through the complete documentation chronologically. if the introduction explains what the component can do and what you need to install to do that, i think it is useful.

i can change this into a sub-paragraph if you think that is better - i felt it being so short an additional title is not necessarily helping. on the other hand, the title will show up in the ToC so it might get useful visibility when starting to look for reasons of the problem.

Copy link
Member

Choose a reason for hiding this comment

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

I understand your point of view. At the same time, I also have to consider the appeal of the documentation — first impressions matter.

If I land on a page that opens with multiple blocks of install commands, bash snippets, and config files, my first thought is often: "Wow, this package looks complicated to install."

But in this case, the vast majority of developers installing UX Icons just need to run composer req symfony/ux-icons, and everything is set up automatically.

I'm not saying we should ignore those who fall outside that category — but I also can't disregard the majority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course, having a 15 step installation instructions is off-putting. but oversimplification is not helping either. i removed the part about ux-twig-component and made a separate subtitle for the http-client

Copy link
Member

Choose a reason for hiding this comment

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

If it's that visible, maybe then we can remove it from the other parts ?

This use case is not the most common, and I'm not 100% convinced it requires multiple big blocks on the same page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

honestly i think if we find a good solution for #2667, we can remove either this section or the ones below in the sections that rely on http client.

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.

(at least for the twig component thing that really does not deserve this highlight)

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Reviewed Has been reviewed by a maintainer labels Mar 30, 2025
@Kocal Kocal changed the title improve setup instructions [Icons] Improve setup instructions Mar 30, 2025
@Kocal
Copy link
Member

Kocal commented Mar 30, 2025

Following https://symfony-devs.slack.com/archives/C01FN4EQNLX/p1743272426739939, I don't remember if this topic was already evoked, but maybe we can improve the DX and always register the commands ux:icons:search and ux:icons:import, but throw at runtime if symfony/http-client is not installed?

@kbond
Copy link
Member

kbond commented Mar 30, 2025

maybe we can improve the DX and always register the commands ux:icons:search and ux:icons:import, but throw at runtime if symfony/http-client is not installed?

That's a great idea!

@dbu
Copy link
Contributor Author

dbu commented Mar 30, 2025

i try to improve the exception when no icon is found and the http client is not available in #2667

with that we could consider leaving the setup instructions as is - although imho its not the greatest DX to install something and then get an error telling i need to install something more.

i missed that i need the http-client installed, the bundle silently reduces functionality without it. make it more clear what needs to be installed
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Mar 31, 2025
@dbu
Copy link
Contributor Author

dbu commented Mar 31, 2025

the two times HTML syntax is introduced

made me realize: the first parts of the section Rendering Icons are repetitions without added information of Loading Icons and the rather randomly appearing HTML Syntax section. i think that is confusing. i suggest to supress the first HTML Syntax section.
Rendering Icons also does not mention attributes anymore, which seems odd to me as it is the "reference" part about rendering.

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

This looks good to me. We can iterate as needed.

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 31, 2025
@@ -166,7 +175,7 @@ HTML Syntax

In addition to the ``ux_icon()`` function explained in the previous sections,
this package also supports an alternative HTML syntax based on the ``<twig:ux:icon>``
tag:
tag if the ``symfony/ux-twig-component`` package is installed:
Copy link
Member

Choose a reason for hiding this comment

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

image

Does feel like a duplicate of the tip 10 lines below.

What about moving it after the introduction sentence ? and before the code examples ?

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 duplicated it on purpose, becaues it is very important, and i think some people will rather see the box, while others rather read the text flow. alternatively we could even put it into the code block as a comment right before the first tag.

which introduction sentence do you mean? the "HTML Syntax" section only has one sentence until here? or you mean moving the ..tip to above the code? i can also do that.

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.

LGTM and we'll improve it if/when we implement an in-code solution

@kbond
Copy link
Member

kbond commented Apr 4, 2025

Thanks David.

@kbond kbond merged commit 17838da into symfony:2.x Apr 4, 2025
2 checks passed
@dbu dbu deleted the patch-1 branch April 7, 2025 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants