[IMPROVEMENTS - PART 1] Global quality improvement #668

tblivet · 2025-01-24T16:31:58Z

Questions	Answers
Description?	This PR includes the first batch of quality improvements and refactoring of the theme, which contains: - Header refactoring, including the integration of the following modules: - `ps_contactinfo` - `ps_searchbar` - `ps_currencyselector` - `ps_languageselector` - `ps_customersignin` - `ps_mainmenu` - `ps_shoppingcart` - The ps_mainmenu desktop display has been completely reworked. - The ps_searchbar functionality has been improved. - The product miniature component has been reworked. - The “product grid” has been updated to use CSS Grid instead of multiple classes. - The workflow has been updated to include Prettier for SCSS files.
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	--
Sponsor company	@PrestaShopCorp
How to test?

src/js/modules/ps_mainmenu.ts

Hlavtox · 2025-01-24T19:27:55Z

Hey @tblivet, I have written most of the code with Valentin, there is one thing I am not really happy about regarding product miniature.

We are doing column count in product list by passing boostrap classes into the templates, but this is just too much work and every module needs to do this.

I have come up with far simplier solution - https://danielhlavacek.cz/products.html, wdyt? It doesn't need to know anything about the context or container. :-)

Hlavtox

Good work, but I don't really like two things:

You are moving a lot of things to core folder and deleting a lot of things from custom folder. The point of this was to separate the basic layout and our own custom design things. So developers can get a nice clean bootstrap base just by commeting out the custom folder. And not having to spend time with deleting things they dont need.
There are container media queries used, I don't think the support is that big. Is it safe to use it?

tblivet · 2025-01-27T10:38:11Z

Hi @Hlavtox 👋 thanks for the review and feedback !

Regarding the grid approach you proposed, I like the solution, but I don't think it’s the best fit for our case. I believe we need contextual control to better manage how many elements are displayed per line. This approach helps us avoid situations where a line is not fully populated with products, as shown in the screenshot below. What do you think?
Yes, in fact, I have started moving files from custom to core because I don’t fully understand the benefit behind this separation. I believe it may lead contributors to confusion about where to write code. I’m not sure what you mean by separating the basic layout from custom design. From my perspective, it’s unclear, but maybe you can help me understand! Currently, when we comment out the import of the custom.scss file, the theme becomes completely non-functional.
I think we can now use container queries, as they are currently supported by over 93% of browsers. Since Hummingbird is intended to be a modern theme and has not yet been officially released, I believe using container queries should be acceptable! We can also implement a fallback using the CSS @supports feature, but I’m not sure if it is necessary in this case, what do you think?

Hlavtox · 2025-01-27T11:42:45Z

Hey @tblivet! :-)

Regarding the grid approach you proposed, I like the solution, but I don't think it’s the best fit for our case. I believe we need contextual control to better manage how many elements are displayed per line. This approach helps us avoid situations where a line is not fully populated with products, as shown in the screenshot below. What do you think?

It does handle this case as well, see here:

With the proposed approach, you don't need to care about layout, about left/right column, nothing. :-) Plus a bonus is that if you decide to widen a layout, new columns will just pop in place.

Yes, in fact, I have started moving files from custom to core because I don’t fully understand the benefit behind this separation. I believe it may lead contributors to confusion about where to write code. I’m not sure what you mean by separating the basic layout from custom design. From my perspective, it’s unclear, but maybe you can help me understand! Currently, when we comment out the import of the custom.scss file, the theme becomes completely non-functional.

Check this article - https://build.prestashop-project.org/news/2022/new-theme-announce/, the point of this separation was, that it allows you to start creating much easier. I think that 90% of theme developers use classic theme as a base for their theme, and if you did this, you maybe spent more time deleting styles you don't want, other than building the new stuff. This way, you can just comment out @import "custom/_custom";, build it and voila, you have somehow clean boostrap layout. :-)

I think we can now use container queries, as they are currently supported by over 93% of browsers. Since Hummingbird is intended to be a modern theme and has not yet been officially released, I believe using container queries should be acceptable! We can also implement a fallback using the CSS @supports feature, but I’m not sure if it is necessary in this case, what do you think?

I don't know what is the current standard, but I think we should aim more for 95 % to 97 % of support, it's ecommerce after all... I have only started using grid/flex gaps recently for example. But, no doubt that it's a cool feature that would allow to nicely style the product miniature sizing for example. :-)

kpodemski · 2025-01-27T12:00:55Z

modules/ps_contactinfo/ps_contactinfo-rich.tpl

      <i class="material-icons" aria-hidden="true">&#xE0CD;</i>
-      <div class="contact__info"><a href="tel:{$contact_infos.phone}">{$contact_infos.phone}</a></div>
+      <div class="ps-contactinfo__info"><a href="tel:{$contact_infos.phone|escape:'htmlall':'UTF-8'}">{$contact_infos.phone|escape:'htmlall':'UTF-8'}</a></div>


What's the point of adding |escape:'htmlall':'UTF-8'?

Hello @kpodemski 👋
This recommendation comes from: https://docs.cloud.prestashop.com/10-validation-checklist/#security
However, I can remove these occurrences in this PR and start a discussion in a separate PR about whether or not to include them in the entire project. Wdyt?

As far as I know, those are still relevant for the back office but not the front office. They were removed on purpose in 1.7 quite a long time ago.

Let me also ping @jolelievre, so he is also aware of this recommendation and share his opinion :)

modules/ps_contactinfo/ps_contactinfo-rich.tpl

modules/ps_contactinfo/ps_contactinfo.tpl

modules/ps_emailsubscription/views/templates/hook/ps_emailsubscription.tpl

kpodemski · 2025-01-27T12:18:05Z

templates/_partials/header.tpl

          <div class="header-block d-flex align-items-center">
            <a class="header-block__action-btn" href="#" role="button" data-bs-toggle="offcanvas" data-bs-target="#searchCanvas" aria-controls="searchCanvas" aria-label="{l s='Show search bar' d='Shop.Theme.Global'}">
              <span class="material-icons header-block__icon">search</span>
            </a>
          </div>

-          <div class="search__offcanvas js-search-offcanvas offcanvas offcanvas-top h-auto" data-bs-backdrop="false" data-bs-scroll="true" tabindex="-1" id="searchCanvas" aria-labelledby="offcanvasTopLabel">
+          <div class="ps-searchbar__offcanvas js-search-offcanvas offcanvas offcanvas-top h-auto" tabindex="-1" id="searchCanvas" aria-labelledby="offcanvasTopLabel">


I don't like the hardcoding module name in the generic file that is header.tpl. What if I change the search module? 🤔

kpodemski · 2025-01-27T12:18:19Z

templates/_partials/header.tpl


+        {* MOBILE SEARCH BAR *}
+        <div class="ps-searchbar--mobile d-md-none d-flex col-auto">


This should be somehow integrated into ps_searchbar module.

kpodemski · 2025-01-27T12:19:21Z

templates/_partials/header.tpl

-              <span class="header-block__badge">{$cart.products_count}</span>
-            </span>
+        {if !$configuration.is_catalog}
+          <div id="_mobile_ps_shoppingcart" class="d-md-none d-flex col-auto">


again, I didn't notice it before, and I think it is indeed just a placeholder for the shopping cart module to load the data from, not just having them hardcoded with the module name there

templates/_partials/header.tpl

templates/catalog/_partials/miniatures/product.tpl

ga-devfront · 2025-01-27T10:53:19Z

modules/ps_contactinfo/ps_contactinfo-rich.tpl

+ <div class="ps-contactinfo">
+  <p class="h2 ps-contactinfo__title">{l s='Store information' d='Shop.Theme.Global'}</p>
+  <div class="ps-contactinfo__item">
+    <i class="material-icons" aria-hidden="true">&#xF00F;</i>


for part 2 it would be interesting to put all the ex code in text so that it remains more meaningful.

ga-devfront · 2025-01-27T11:02:32Z

modules/ps_shoppingcart/ps_shoppingcart.tpl

+  <div class="ps-shoppingcart">
+    <div class="header-block d-flex align-items-center blockcart cart-preview {if $cart.products_count> 0}header-block--active{else}inactive{/if}" data-refresh-url="{$refresh_url}">
+      {if $cart.products_count> 0}
+        <a class="header-block__action-btn pe-md-0" rel="nofollow" href="{$cart_url}" aria-label="{l s='View cart (%d products)' d='Shop.Theme.Checkout' sprintf=[$cart.products_count]}">


I think that can be a cool idea to escape $cart_url to prevent side effect

what do you mean? front office variables are automatically escaped

kpodemski

I am blocking to prevent accidental merge.

tblivet · 2025-01-27T17:24:57Z

Hi @Hlavtox :)

Sorry for the confusion, but this scenario is exactly what I wanted to avoid. A line of products that isn't fully populated can make the layout feel a bit buggy. However, I agree that an approach that doesn't require consideration of the page layout is beneficial, but in this case, I believe it’s not applicable. For example, by default, the module displays 8 products, so having 3 products per line on certain resolutions isn’t visually appealing, as it results in a row with only one product. A better alternative could be adding a carousel for modules that display products on the homepage, but that’s not the focus here.

It does handle this case as well, see here:

Ok, but currently, when we comment out the @import "custom/_custom"; file, the result on the develop branch doesn’t look like a clean Bootstrap layout to me. It may have been the case at the beginning of the project, but I don’t think so anymore?
Homepage screenshot without custom.scss:

Check this article - https://build.prestashop-project.org/news/2022/new-theme-announce/, the point of this separation was, that it allows you to start creating much easier. I think that 90% of theme developers use classic theme as a base for their theme, and if you did this, you maybe spent more time deleting styles you don't want, other than building the new stuff. This way, you can just comment out @import "custom/_custom";, build it and voila, you have somehow clean boostrap layout. :-)

From my point of view, this is acceptable, as customers generally use modern smartphones or laptops and are not stuck with outdated browsers, as is often the case in some companies. However, if anyone thinks it’s too early, I can implement a CSS @supports fallback for this feature.

I don't know what is the current standard, but I think we should aim more for 95 % to 97 % of support, it's ecommerce after all... I have only started using grid/flex gaps recently for example. But, no doubt that it's a cool feature that would allow to nicely style the product miniature sizing for example. :-)

kpodemski · 2025-01-27T17:53:49Z

Ok, but currently, when we comment out the @import "custom/_custom"; file, the result on the develop branch doesn’t look like a clean Bootstrap layout to me.

Although it doesn't look like a clean design, it is a clean bootstrap, and that was its purpose: to serve as a skeleton for developers who dislike the default styling.

Hlavtox · 2025-01-27T18:00:26Z

@tblivet

Sorry for the confusion, but this scenario is exactly what I wanted to avoid. A line of products that isn't fully populated can make the layout feel a bit buggy.

It's perfectly normal to me, in a real world, it just happens all the time. What would you want to do instead, make one product 3 columns wide? :-)

However, I agree that an approach that doesn't require consideration of the page layout is beneficial, but in this case, I believe it’s not applicable. For example, by default, the module displays 8 products, so having 3 products per line on certain resolutions isn’t visually appealing, as it results in a row with only one product. A better alternative could be adding a carousel for modules that display products on the homepage, but that’s not the focus here.

I don't understand, I am keeping the same behavior as now - 3 products per row in left column layouts, 4 products per row in full width layouts. :-) If people don't want to have 1 products per line, they need to make sure to implement some conditions in their modules.

Ok, but currently, when we comment out the @import "custom/_custom"; file, the result on the develop branch doesn’t look like a clean Bootstrap layout to me. It may have been the case at the beginning of the project, but I don’t think so anymore?
Homepage screenshot without custom.scss:

Yeah, it doesn't look good. I wasn't active in this repo for a while. The original idea was to make it look something like Falcon - https://falcon-theme.dev/en/.

kpodemski · 2025-01-28T09:48:08Z

@tblivet what about some other comments I made? :) if you want to move them to another PR then fine, but I think we lack of consistency with {$componentName} and hardcoded module names in the header are not good for the overall quality of the theme

tblivet · 2025-01-28T10:00:24Z

@kpodemski I completely agree that a module should be handled through hooks and not directly inside the theme .tpl files, but that's not currently the case. What I’m proposing in this PR is a "Part 1" of the optimization. I believe there’s still a lot to do to improve the overall quality, but we can’t include everything in this single PR ?

@tblivet what about some other comments I made? :) if you want to move them to another PR then fine, but I think we lack of consistency with {$componentName} and hardcoded module names in the header are not good for the overall quality of the theme

modules/ps_languageselector/ps_languageselector.tpl

Hlavtox · 2025-01-28T10:54:28Z

templates/catalog/_partials/miniatures/product.tpl

    data-id-product="{$product.id_product}"
    data-id-product-attribute="{$product.id_product_attribute}"
  >
-    <div class="card">


Why do you remove all of the things we spent so much time on? The usage of card and card-body had their purpose, to automatically style it somehow...

that's true, it follows what has been done in Falcon and what Daniel mentioned before, if you "skip" the "custom" layer you will end up with a design that looks like a clean Boostrap theme

@Hlavtox I have removed this because, in my opinion, product-miniature appears to be more of a fully custom component rather than a card component.

@kpodemski I understand, but currently, we don’t ended up with a clean Bootstrap theme. By "clean," you mean an unmodified Bootstrap version?

I believe this concept wasn’t clearly defined and shared among all contributors. If we look at the current implementation, if I understand well, we can see elements that don’t align with this vision?

Example: Breadcrumb Component

Some SCSS variables from the Bootstrap breadcrumb are overridden in abstract/variables/bootstrap, which is good.

Then, we have core/components/_breadcrumb.scss, which seems to adapt the breadcrumb to our styling needs.

Finally, there's custom/components/_breadcrumb.scss, which, in this case, only contains margin: 0. However, I’m not sure if this is expected ? Should custom/components only contain components unrelated to Bootstrap?

Another Example: Product Miniature

We have styles in core/components/product/_product-miniature.scss, which are related to custom classes defined in the template that handle product miniature.

We also have styles in custom/components/product/_product-miniature.scss, which again relate to custom classes in the same template.

In this case, I don’t really understand why both exist. Is there a specific reason for this? Or is it just, as I mentioned in another comment, that contributors are confused or unsure where to contribute?

@tblivet Unfortunately, as with everything Prestashop SA invests in, the work was not finished and it was abandoned. @NeOMakinG left the company and I lost the will to work on it for free.

So yeah, I agree with you, it doesn't look like it now, but that was the vision. To provide developers with easy starter template. Something that will only contain the most basic styles.

So, for example, after commenting out the custom folder, you should get something like this https://getbootstrap.com/docs/5.3/examples/checkout/.

Is it a style that makes the layout and makes thing work? = core/

Is it a style that adds colors, rounded borders, margins, paddings, fonts etc.? = custom/

@Hlavtox Thank you for this information, it's a bit clearer what was trying to be achieved. I think it's a great idea to provide developers with a clean base Bootstrap theme 👍 I also believe the design should have been entirely based on Bootstrap to achieve this, and I think it wasn't the case 😕

However, there's one thing I don't agree with: the use of core/ and custom/.

I strongly believe that it creates confusion and slows down the integration process. Manually splitting layout and visual styles into two separate folders seems a bit inconvenient, doesn’t it?

Maybe core/ should have been named bootstrap/ to specifically style Bootstrap components, while custom/ would be used only for PrestaShop specific custom components ?

I think @tblivet 's solution is an excellent idea to clarify this part.

What about structure and styling folders?

tblivet added 17 commits January 16, 2025 17:17

feat: refacto WIP

6a4f1d9

feat: refacto WIP

77b0a20

feat: refacto WIP

65c31ce

feat: refacto WIP

ddfe295

feat: refacto WIP

c831af8

fix: eol

e1ab2d3

feat: refacto WIP

e2b7022

feat: adjust container query

05070d3

feat: refacto WIP

c147f43

feat: refacto WIP

2d7f6d9

feat: refacto WIP

89dddd8

feat: add prettier on SCSS

6291261

chore: lint

5f7efa7

chore: update workflow

31145ba

feat: refacto WIP

1feb248

chore: update workflow

08073b3

chore: lint

55a9244

tblivet requested review from kpodemski and ga-devfront January 24, 2025 16:35

Hlavtox reviewed Jan 24, 2025

View reviewed changes

src/js/modules/ps_mainmenu.ts Outdated Show resolved Hide resolved

Hlavtox requested changes Jan 24, 2025

View reviewed changes

ps-jarvis added the Waiting for author label Jan 24, 2025

feat: improvements + fix review

9edd52d

tblivet requested a review from Hlavtox January 27, 2025 10:38

tblivet removed the Waiting for author label Jan 27, 2025

kpodemski reviewed Jan 27, 2025

View reviewed changes

ga-devfront previously approved these changes Jan 27, 2025

View reviewed changes

ps-jarvis added the Waiting for QA label Jan 27, 2025

kpodemski requested changes Jan 27, 2025

View reviewed changes

ps-jarvis added the Waiting for author label Jan 27, 2025

tblivet dismissed ga-devfront’s stale review via 0cc0298 January 27, 2025 17:44

fix: smarty condition

3d91468

tblivet force-pushed the feat/global-quality-improvements branch from 0cc0298 to 3d91468 Compare January 27, 2025 17:47

tblivet requested a review from kpodemski January 28, 2025 09:03

fix: remove escape html

4af6d8b

Hlavtox reviewed Jan 28, 2025

View reviewed changes

modules/ps_languageselector/ps_languageselector.tpl Outdated Show resolved Hide resolved

Hlavtox reviewed Jan 28, 2025

View reviewed changes

tblivet added 3 commits January 28, 2025 14:15

fix: replace iso code by named language

92948a5

feat: remove mobile searchbar from header template

ed79c63

fix: icon width and height

3fee3fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IMPROVEMENTS - PART 1] Global quality improvement #668

[IMPROVEMENTS - PART 1] Global quality improvement #668

tblivet commented Jan 24, 2025

Hlavtox commented Jan 24, 2025

Hlavtox left a comment

tblivet commented Jan 27, 2025

Hlavtox commented Jan 27, 2025 •

edited

Loading

kpodemski Jan 27, 2025

tblivet Jan 27, 2025

kpodemski Jan 27, 2025

kpodemski Jan 27, 2025

kpodemski Jan 27, 2025

kpodemski Jan 27, 2025

ga-devfront Jan 27, 2025

ga-devfront Jan 27, 2025

kpodemski Jan 27, 2025

kpodemski left a comment

tblivet commented Jan 27, 2025

kpodemski commented Jan 27, 2025

Hlavtox commented Jan 27, 2025

kpodemski commented Jan 28, 2025

tblivet commented Jan 28, 2025

Hlavtox Jan 28, 2025

kpodemski Jan 28, 2025

tblivet Jan 29, 2025

Hlavtox Jan 29, 2025

tblivet Jan 31, 2025

ga-devfront Jan 31, 2025

Hlavtox Jan 31, 2025


		{* MOBILE SEARCH BAR *}
		<div class="ps-searchbar--mobile d-md-none d-flex col-auto">

[IMPROVEMENTS - PART 1] Global quality improvement #668

Are you sure you want to change the base?

[IMPROVEMENTS - PART 1] Global quality improvement #668

Conversation

tblivet commented Jan 24, 2025

Hlavtox commented Jan 24, 2025

Hlavtox left a comment

Choose a reason for hiding this comment

tblivet commented Jan 27, 2025

Hlavtox commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodemski left a comment

Choose a reason for hiding this comment

tblivet commented Jan 27, 2025

kpodemski commented Jan 27, 2025

Hlavtox commented Jan 27, 2025

kpodemski commented Jan 28, 2025

tblivet commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example: Breadcrumb Component

Another Example: Product Miniature

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hlavtox commented Jan 27, 2025 •

edited

Loading