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

navigation/dashboard.phtml: Use ipl-web's Icon for fa- icons #5278

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

navigation/dashboard.phtml: Use ipl-web's Icon for fa- icons #5278

wants to merge 2 commits into from

Conversation

nilmerg
Copy link
Member

@nilmerg nilmerg commented Oct 7, 2024

resolves #5277

@nilmerg nilmerg added enhancement New feature or improvement area/framework Affects third party integration/development area/ui Affects the user interface labels Oct 7, 2024
@nilmerg nilmerg added this to the 2.12.2 milestone Oct 7, 2024
@nilmerg nilmerg requested a review from raviks789 October 7, 2024 14:26
@nilmerg nilmerg self-assigned this Oct 7, 2024
@cla-bot cla-bot bot added the cla/signed label Oct 7, 2024
@nilmerg
Copy link
Member Author

nilmerg commented Oct 7, 2024 •
edited
Loading

requires Icinga/ipl-web#232
without it, the result is the same as without this change

@moreamazingnick
Copy link
Contributor

since the branch is called fa-icons for everyone...
does this fit into that?
#5205

@nilmerg
Copy link
Member Author

nilmerg commented Oct 8, 2024

In theory, for menu items, yes. Patching url.php makes not much sense to me, it's legacy code and links with fontawesome icons are constructed differently nowadays.

raviks789
raviks789 previously approved these changes Oct 8, 2024
View reviewed changes
@nilmerg
url.php: Add partial support for fontawesome
f599035 
Partial because the helper method is not the preferred way
anymore to create an icon. So I simplified the detection
to only check whether the given icon is a legacy one, as
those are smaller in number. Though, this leads to some fa
icons being identified as legacy, as the names equal. But,
it's the legacy helper after all... Anyone wanting to make
sure to get fontawesome icons, must add the `fa-` prefix.
@nilmerg
Copy link
Member Author

nilmerg commented Oct 10, 2024

@moreamazingnick Now it does fit.
@raviks789 I've added the map as discussed recently.

@nilmerg nilmerg linked an issue Oct 10, 2024 that may be closed by this pull request
Feature Request / allow fontawsome icons as menu items #5205
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raviks789 raviks789 raviks789 approved these changes

Assignees

@nilmerg nilmerg

Labels
area/framework Affects third party integration/development area/ui Affects the user interface cla/signed enhancement New feature or improvement
Projects
None yet
Milestone
2.12.2
3 participants
@nilmerg @moreamazingnick @raviks789