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

is column width percentage necessary? #100

Closed
proseLA opened this issue Nov 29, 2021 · 10 comments
Closed

is column width percentage necessary? #100

proseLA opened this issue Nov 29, 2021 · 10 comments
Labels
change New feature or request fix provided A correction has been provided
Milestone

Comments

@proseLA
Copy link

proseLA commented Nov 29, 2021

if ($product_listing_layout_style === 'columns') {
$style = ' style="width:' . $col_width . '%;"';
}

i think these lines are a problem. i have PRODUCT_LISTING_COLUMNS_PER_ROW set to 2, which works fine on a desktop, but when i move to mobile view (375 x 812), the width is then hard coded at 49.5% and i'm only seeing 1 item per row. it just has not expanded itself out to the full width.

the number of products in my listing is at 5.

i think hard coding width percentages is a mistake. i'm curious if anyone else can replicate the problem or if it is just me.

@lat9
Copy link
Owner

lat9 commented Nov 30, 2021

I agree that this element of the layout needs 'reworking'. The best way to configure the layout is to set PRODUCT_LISTING_COLUMNS_PER_ROW to either '0' (fluid columns) or '1' (rows). The 'fluid columns' are then calculated via this

        // set css classes for "row" wrapper, to allow for fluid grouping of cells based on viewport
        // these defaults are based on Bootstrap4, but can be customized to suit your own framework
        if ($product_listing_layout_style === 'fluid') {
            $grid_cards_classes = 'row-cols-1 row-cols-md-2 row-cols-lg-2 row-cols-xl-3';
            // this array is intentionally in reverse order, with largest index first
            $grid_classes_matrix = [
                '10' => 'row-cols-1 row-cols-md-2 row-cols-lg-4 row-cols-xl-5',
                '8' => 'row-cols-1 row-cols-md-2 row-cols-lg-3 row-cols-xl-4',
                '6' => 'row-cols-1 row-cols-md-2 row-cols-lg-2 row-cols-xl-3',
            ];

section.

I'm considering elevating that 'matrix' into (yet another) configuration setting, where the xs/sm/md/lg/xl columns/row are specified.

@proseLA
Copy link
Author

proseLA commented Nov 30, 2021

rows are not what i would call responsive on a mobile device.

setting it to fluid, while it looks good on mobile, does not work in my setup on desktop as the resulting matrix is just really off. in my setup, 12 columns are shown.

further investigation demonstrates that the removal of the code on lines 417-419 does NOT affect the display with either a setting of 0 or 1 on either viewport tested. with a setting of 2 the viewport on a mobile device is cleaned up, and on the desktop unchanged.

so while i agree that the matrix does need reworking (as demonstrated by the fluid setting on desktop), i would submit that those lines of code hard coding the width are still unneeded.

@lat9
Copy link
Owner

lat9 commented Nov 30, 2021

I never said that rows are responsive. If your customization uses a 12-across columns to display the product listings, you can modify your template's override of the product_listing.php, adding the following as the first element to that $grid_classes_matrix:

                '12' => 'row-cols-1 row-cols-sm-2 row-cols-md-3 row-cols-lg-4 row-cols-xl-6',

@proseLA
Copy link
Author

proseLA commented Nov 30, 2021

i will take a look.

at this point, i have not customized anything. i do not want a 12-across. when i set the constant to 0 (ie, fluid), a 12 across was shown in my desktop while a single was shown in my mobile. i only looked at 2 different viewports.

how the template got to 12, i have not dug into it, but it is not want i wanted. and it does not look good.

getting back to my original point which is i do not think we should be hard coding a width inside of a style tag. i'm a bigger fan of just using classes.

@lat9
Copy link
Owner

lat9 commented Nov 30, 2021

How do you have the various column-widths set?
image

I'm thinking about removing support for PRODUCT_LISTING_COLUMNS_PER_ROW for any value other than '0' (fluid) or '1' (rows) and providing viewport-width-specific settings for the row-cols-* values.

@lat9
Copy link
Owner

lat9 commented Nov 30, 2021

That gets complicated, since specific pages can disable either the left- or right-sideboxes (or both). Using the above column-width settings, for instance, the center column (where the products' listings are rendered) can be either 6, 9 or 12 columns wide.

@lat9
Copy link
Owner

lat9 commented Nov 30, 2021

After further review, any change here has the possibility of disrupting current installations' settings on an upgrade. To mitigate further issues, I'm considering:

  1. Updating the description for PRODUCT_LISTING_COLUMNS_PER_ROW to indicate that the values of '0' (fluid) and '1' (rows) are the preferred settings for the bootstrap template.
  2. Updating the $grid_classes_matrix to also include 'suggestions' for center-column widths of '12' and '9'.

@proseLA
Copy link
Author

proseLA commented Nov 30, 2021

How do you have the various column-widths set? image

mine were set exactly the same as the widths in the image.

@proseLA
Copy link
Author

proseLA commented Nov 30, 2021

ok, i'm not exactly sure what happened. but i can not reproduce the 12 across with it set to fluid on my desktop.

i updated a bunch of the template files to the latest from this repo, and now i'm getting much better results with it set to fluid.

if it is set to 2, then the width problem comes in on mobile, as well as it not looking good on desktop.

so i am a fan of either 0 or 1. i think fluid is the way to go... although i still stand by the no need for width; but i think a minor point.

feel free to close the issue, unless you want it open.

best.

lat9 added a commit that referenced this issue Dec 1, 2021
@lat9 lat9 added the change New feature or request label Dec 1, 2021
@lat9 lat9 added this to the v3.2.0 milestone Dec 1, 2021
@lat9 lat9 added the fix provided A correction has been provided label Dec 1, 2021
@lat9
Copy link
Owner

lat9 commented Dec 3, 2021

I'll also update the product_listing.php module to enable site-specific customizations of the $grid_classes_matrix without modification to that bootstrap-template-supplied module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change New feature or request fix provided A correction has been provided
Projects
None yet
Development

No branches or pull requests

2 participants