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

POC: theme icons map #551

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Conversation

Oksydan
Copy link
Contributor

@Oksydan Oksydan commented Sep 28, 2023

Questions Answers
Description? Simple POC replacing plain icon fonts with icons map and simple renderIcon smarty function. It should allow developers to adjust their icons in themes and allow module developers to use icons w/o being aware of what icons are being used by theme. Icons map should be standardized in some way 🥶
Type? POC
BC breaks? no
Deprecations? no
Fixed ticket? https://github.com/PrestaShop/hummingbird/discussions/549
Sponsor company Waynet
How to test? N/A

@Oksydan Oksydan changed the title POC: theme icon map POC: theme icons map Sep 28, 2023
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Icons map should be standardized in some way

I think indeed it's important to make it very clear how to build / provide this icon map else the feature can create confusion or bad usage

{if $iconName && !empty($iconsMap[$iconName])}

<span
class="material-icons {if !empty($extraAttributes['class'])}{$extraAttributes['class']}{/if}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we have material-icons hardcoded so what would happen if someone wanted to use another icon lib? They override this smarty template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matks thank you for your response.
Tbh modules aren't aware what icon fonts theme use. It's theme responsibility do display proper icon to given icon key. If you would like to change icon fonts to for example font-awsome, all you have to do is changing material icons to fa and map keys to your new icon-fonts.
Instead of:

{assign
  var=iconsMap
  value=[
    "cart" => "shopping_cart"
  ]
  scope="global"
}

for example:

{assign
  var=iconsMap
  value=[
    "cart" => "fa-shopping-cart" // not sure it's valid icon class
  ]
  scope="global"
}

@SharakPL
Copy link
Contributor

SharakPL commented Oct 15, 2023

Looks like svgs are defined in 2 places: icons-sprite.svg and helpers.tpl partial. I'm not sure this is a good idea. There should be 1 source for the icon. Also why don't you let the SVG be compiled and minified? IMO /assets should contain only compiled resources.

@Oksydan
Copy link
Contributor Author

Oksydan commented Oct 15, 2023

Hi @SharakPL
It's only for POC. It's not ready solution.

@tblivet
Copy link
Contributor

tblivet commented Jul 18, 2024

Thank you, @Oksydan. I think this proof of concept is a good idea. 👍

From my point of view, I'm not sure that using an icon map is a good idea. If we want to standardize this, we should not test if the iconName is part of the iconsMap, because this implies adding new icons that don’t appear on the map is impossible (for example, if a module dev wants to use a new icon).

For the SVG sprite, the idea is very interesting. This would make the use of SVG sprites much easier.

Finally, I want to point out a problem with this POC that I think can't be solved within the theme. In your example, you change the shopping_cart icon, and this icon is called inside a template which can be refreshed if we add a product to the cart. So, when we add a product to the cart, the "add to cart" modal doesn't show up because the function defined in Smarty .tpl is not registered globally, I suppose. This triggers an error and breaks the JS. Therefore, we can't use the stuff you propose inside .tpl files that are refreshed by JS (I think about multiple .tpl files call inside the product page for example). I think this can only be solved by adding a module in the theme that registers the Smarty function, or by adding this to the core in php. What do you think about that?

Copy link
Contributor

@tblivet tblivet left a comment

Choose a reason for hiding this comment

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

PR need to work on refreshed elements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

5 participants