Closed kpodemski closed 1 week ago
Yes, we should use a flag called framework
.
This flag could be accessible out of the box by calling $this->context->shop->theme->get('framework')
.
The only issue I see, is that the core is loading the config from the parsed json file in config/themes
folder, which is not updated when updating the theme.
We could probably parse the YML right away to get the right data, but there are two issues with it:
0.07ms
from JSON, 1.38ms
from YML. However, in case of one call, 0.0001ms
and 0.0036ms
is no difference.Second issue is that if you configure page layouts in backoffice, they get saved into this JSON. We will need to come up with a different place to store the layout config.
IMHO this is completely unrelated to the main point of this issue, which is the ability to distinguish bs5-based themes from bs4-based themes 👍🏻
@kpodemski I expanded on that topic because of
The only issue I see, is that the core is loading the config from the parsed json file in config/themes folder, which is not updated when updating the theme.
So if you update a theme to use Boostrap 5 to Bootstrap 6, core will still tell you that this theme is using v5.
@kpodemski Something like this? https://github.com/PrestaShop/classic-theme/pull/149
It works using $this->context->shop->theme->get('meta.framework')
One important point still left @matks @jolelievre, the core is caching the theme YML inside config/themes/themename/shopX.json
.
@Hlavtox
is that an issue tho? you won't change this flag after you switch the theme, and cache is cleared after changing it
@kpodemski What if you update the theme by uploading a new zip? (This is how would I do it.)
@Hlavtox then the cache of the theme should be cleared 🤔 although, I remember that sometimes it has worked like you would "reset theme" and mess up people's layout configurations, so... something to double-check
@kpodemski The problem is that page layout configurations are saved in that JSON, so when you clear it, it's fucked :D
Hi guys here is the PR that introduces the modal based on the theme config https://github.com/PrestaShop/PrestaShop/pull/36731
The criteria to show the modal is if the theme IS bootstrap and is not version4 (maybe I should check if >4
instead 🤔) The wording is not validated yet, and especially the doc URL is not correct For now it points to https://devdocs.prestashop-project.org/9/themes/hummingbird/ but it should be a dedicated page for bootstrap5 (with info about the compatibility layer)
Regarding the cache in json file, this is indeed an issue, I had the problem when I tested locally because I already had these files generated Although it's a different topic IMHO because the problem shouldn't happen for fresh installs (because the cache was never generated) and in most of the upgrades (because the hummingbird was not initially included).
The cases where this bootstrap feature would fail is if the shop already installed the hummingbird theme previously, . It doesn't mean we don't have an issue regarding this cache but it's a topic on its own.
It would be good to change the storage for the layout settings, and introduce some hooks, maybe?
It's very awkward when you have custom controllers in a module and want to setup some layout by default, you have to modify the json file manually with some hackjobs.
@Hlavtox I don't know the layout system well enough to grasp everything, but yeah I guess this could be improved This feature is mixing cache with configuration but it doesn't naturally inherit from changes due to updates It could probably be improved simply:
Regarding the hook you suggest I'd need more details to understand the way it works and the drawbacks.
This subject is finished even though some changes might come with https://github.com/PrestaShop/PrestaShop/issues/36727
We should add a flag to theme.yml and to the core, allowing the developer to check whether the theme is based on Bootstrap5. It is to distinguish the use of Classic vs. Hummingbird as a base of the theme. Why? Because it impacts front-end work from those developers.
I'm pretty sure I discussed it with @MatShir and @Hlavtox as an absolute must-have, but I cannot find an issue about it.