Skip to content

[Icons] Some doc updates #1602

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
Merged
merged 1 commit into from
Mar 25, 2024
Merged

[Icons] Some doc updates #1602

merged 1 commit into from
Mar 25, 2024

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Mar 9, 2024

Step by step...

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Mar 9, 2024
@javiereguiluz javiereguiluz mentioned this pull request Mar 21, 2024
Merged
javiereguiluz
javiereguiluz requested changes Mar 21, 2024
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Fantastic contribution Simon!

I left some comments while reviewing it. You don't have to do ALL requested changes. Let's discuss about them if you disagree. Thanks!

src/Icons/doc/index.rst Outdated
------------

The UXIcon component is designed to be as fast as possible, while offering a
great deal of flexibility. The following are some of the optimizations made to
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the mention to "flexibility". Let's focus on saying that UX Icons is fast and how is that possible.

src/Icons/doc/index.rst
TwigComponent
~~~~~~~~~~~~~

The ``ux_icon`` function is optimized to be as fast as possible. To deliver the same level
Copy link
Member

Choose a reason for hiding this comment

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

is optimized to be as fast as possible -> is optimized for speed

src/Icons/doc/index.rst
</svg>


Configuration
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 we don't need this section because we already show that in other part of this doc.

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Mar 21, 2024
@smnandre
Copy link
Member Author

Thank you very much @javiereguiluz ! I'll look in depth tonight :)

kbond added a commit that referenced this pull request Mar 21, 2024
@kbond
minor #1641 [Doc] Tweaks and fixes for UX Icons docs (javiereguiluz)
cb57b81 
This PR was merged into the 2.x branch.

Discussion
----------

[Doc] Tweaks and fixes for UX Icons docs

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

I think we should mention some of the popular icon sets so people googling for them and Symfony can find this page. Also, let's show the config file path in the config example. I'm not sure if we want that file path to be `config/packages/ux_icons.yaml`

In #1602 we're making more changes to docs, so, if this PR is merged, I think it should be merged before #1602. Thanks!

Commits
-------

e3ac0a2 [Doc] Tweaks and fixes for UX Icons docs
@smnandre
Copy link
Member Author

Fantastic contribution Simon!

I left some comments while reviewing it. You don't have to do ALL requested changes. Let's discuss about them if you disagree. Thanks!

Thank you for the kind words! I have a big rebase to do first, and we'll see then :)) But if i miss one it's not volontary :)

@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Mar 21, 2024
@smnandre
Copy link
Member Author

@javiereguiluz @kbond i let you fix / merge change if you want, i won't have time next days/week :(

I'va added a list of main packages with prefix and links too

@smnandre
Copy link
Member Author

I probably made some mistake, the rebase has been an epic fight (that i lost 😓 )

@norkunas
Copy link
Contributor

the rebase has been an epic fight (that i lost 😓 )

from my experience it's way easier to rebase when you have a single commit only :)

@smnandre
Copy link
Member Author

from my experience it's way easier to rebase when you have a single commit only :)

The challenge here was to manage a file that had been deeply modified by two separate pull requests (PRs).

In reality, this shouldn't have been a problem, and it was a mistake on my part to work on this file at the same time as @kbond 😄

@smnandre smnandre requested a review from javiereguiluz March 24, 2024 21:48
javiereguiluz
javiereguiluz reviewed Mar 25, 2024
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Good job Simon!

I left some comments, but many of them are really minor. Thanks!

@smnandre smnandre requested a review from javiereguiluz March 25, 2024 08:10
javiereguiluz
javiereguiluz approved these changes Mar 25, 2024
View reviewed changes
Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

Very nice! 👏 Thanks!

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 25, 2024
kbond
kbond reviewed Mar 25, 2024
View reviewed changes
@smnandre smnandre force-pushed the icons/doc-readme branch 2 times, most recently from 664629e to f6523cf Compare March 25, 2024 21:15
@smnandre smnandre requested a review from kbond March 25, 2024 21:19
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Reviewed Has been reviewed by a maintainer labels Mar 25, 2024
@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Mar 25, 2024
@kbond kbond force-pushed the icons/doc-readme branch from ce81b95 to eb585b5 Compare March 25, 2024 21:33
@kbond
Copy link
Member

kbond commented Mar 25, 2024

Thanks Simon.

@kbond kbond merged commit 394afee into symfony:2.x Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz approved these changes

@kbond kbond kbond approved these changes

Assignees
No one assigned
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smnandre @norkunas @kbond @javiereguiluz @carsonbot