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

a few small fixes that have also been made in the classic theme in progress #407

Conversation

JBLach
Copy link
Contributor

@JBLach JBLach commented Dec 16, 2022

Questions Answers
Description? Fixes for native modules
Type? improvement
Fixed ticket? #255

Module ps_brandlist -> needs to be styled and updated to bootstrap 5 I'm working on it
Module ps_supplerlist -> needs to be styled and updated to bootstrap I'm working on it
image

Module productcomments -> improve the appearance of buttons in the modal when we want to submit a comment
I'm working on it
image

Module ps_emailalerts -> improve the view on the user's page

Update

ps_brandlist, ps_supplerlist

image
image

productcomments

image

ps_emailalerts

from

image

to

image

@JBLach JBLach changed the title a few small fixes that have also been made in the classic theme a few small fixes that have also been made in the classic theme in progress Dec 16, 2022
@JBLach JBLach marked this pull request as draft December 16, 2022 11:59
@JBLach
Copy link
Contributor Author

JBLach commented Dec 16, 2022

What do you think about changing brand and supplier pages to left column instead of full width or increasing product columns to 4 per row

now

image

4 columns

image

left-column

image

@kpodemski kpodemski assigned JBLach and unassigned JBLach Dec 19, 2022
@JBLach
Copy link
Contributor Author

JBLach commented Dec 19, 2022

@NeOMakinG @Hlavtox what do you think about my changes and comments? :)

@JBLach JBLach marked this pull request as ready for review December 19, 2022 08:54
@NeOMakinG
Copy link
Contributor

@JBLach awesome, I'm going to review this right now

For the question, I would opt for the version with the left column but we need to make it validated by the @PrestaShop/product-team

Copy link
Contributor

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

Some small comments but globally looks really cool, awesome job @JBLach 🍬

modules/ps_brandlist/views/templates/hook/ps_brandlist.tpl Outdated Show resolved Hide resolved
modules/ps_shoppingcart/modal.tpl Show resolved Hide resolved
modules/ps_shoppingcart/modal.tpl Show resolved Hide resolved
templates/catalog/brands.tpl Outdated Show resolved Hide resolved
templates/catalog/listing/manufacturer.tpl Outdated Show resolved Hide resolved
templates/catalog/listing/supplier.tpl Outdated Show resolved Hide resolved
templates/catalog/suppliers.tpl Outdated Show resolved Hide resolved
@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 19, 2022

@JBLach Love everything, exceptional job dude.

As for the columns > We should automatically change from 3 to 4 product grid as you did, depending on left column being enabled or not.

And a personal taste, I would also enable left columns on brand/supplier pages by default. 👍

@eshraw
Copy link

eshraw commented Dec 19, 2022

@JBLach awesome, I'm going to review this right now

For the question, I would opt for the version with the left column but we need to make it validated by the @PrestaShop/product-team

Hello thanks @JBLach for the PR, great job !

Let's go with the left column option, it will be better for navigation and clarity !

@NeOMakinG
Copy link
Contributor

@JBLach @eshraw is from the product team, you can follow his choice :)

Take the @Hlavtox comment in account about the columns sizes being adapted depending on the layout used

@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 19, 2022

On my theme (based on classic-rocket), it's done by the body class - layout-full-width, layout-left-column etc.

@JBLach
Copy link
Contributor Author

JBLach commented Dec 19, 2022

On my theme (based on classic-rocket), it's done by the body class - layout-full-width, layout-left-column etc.

Hi, @Hlavtox how exactly would you do it, i thought about something like this

{capture assign="productClasses"}
  {if !empty($productClass)}
    {$productClass}
  {else}
    {foreach from=$page.body_classes item="class" key="layout"}
      {if $layout === 'layout-full-width'}
        {if $class}
          col-xs-6 col-xl-3
        {else}
          col-xs-6 col-xl-4
        {/if}
      {/if}
    {/foreach}
  {/if}
{/capture}

but it's probably a bad solution

@NeOMakinG NeOMakinG linked an issue Dec 19, 2022 that may be closed by this pull request
@NeOMakinG
Copy link
Contributor

On my theme (based on classic-rocket), it's done by the body class - layout-full-width, layout-left-column etc.

Hi, @Hlavtox how exactly would you do it, i thought about something like this

{capture assign="productClasses"}
  {if !empty($productClass)}
    {$productClass}
  {else}
    {foreach from=$page.body_classes item="class" key="layout"}
      {if $layout === 'layout-full-width'}
        {if $class}
          col-xs-6 col-xl-3
        {else}
          col-xs-6 col-xl-4
        {/if}
      {/if}
    {/foreach}
  {/if}
{/capture}

but it's probably a bad solution

@Hlavtox could you give your advice there? I would accept his solution as it looks sane even if it could be a bit more systemic, we could have an array or key/value pair that we use like $classes[$layout]

@JBLach JBLach force-pushed the verify-the-compatibility-with-natives-modules branch from 6595daa to aea197c Compare December 21, 2022 09:57
@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 21, 2022

@NeOMakinG @JBLach Guys, I am still wondering if the thing with product classes passed to the TPL was a right solution, it just feels complicated.

I just think it should be a

div .product-grid
   div. product-miniature
   div. product-miniature
   div. product-miniature

and do the sizing automatically. We can use the body classes to determine the layout and just behave according to it.

We can call about it. What is opinion of @SharakPL and @Oksydan?

@NeOMakinG
Copy link
Contributor

NeOMakinG commented Dec 21, 2022

@SharakPL wanted to rework it with grids but never started it, and I gave it a go in the past

Anyway, it shouldn't be blocking this PR at all

@SharakPL
Copy link
Contributor

I still plan to make full remake of product list with grid/list switch included. But I'm going through covid 2nd time in last couple months so that's another delay. Sorry guys 😭 I hope to get it done in January...

@NeOMakinG
Copy link
Contributor

I still plan to make full remake of product list with grid/list switch included. But I'm going through covid 2nd time in last couple months so that's another delay. Sorry guys 😭 I hope to get it done in January...

Yeah don't worry, but this means we can accept @JBLach PR regardless of that system

@Oksydan
Copy link
Contributor

Oksydan commented Jan 4, 2023

@Hlavtox I don't like relaying on body classes.
Inside template just check if $layout !== 'layouts/layout-full-width.tpl' and pass different $productClasses to product.tpl.

@kpodemski kpodemski added this to the Beta milestone Jan 5, 2023
@kpodemski kpodemski merged commit a514114 into PrestaShop:develop Jan 5, 2023
@kpodemski
Copy link
Contributor

Thanks @JBLach 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verify the compatibility with every natives modules
7 participants