soflyy / oxygen-bugs-and-features

Bug Reports & Feature Requests for Oxygen
https://oxygenbuilder.com/
314 stars 29 forks source link

Major flaw in repeater's design: pagination div is part of the items list! #959

Open yankiara opened 4 years ago

yankiara commented 4 years ago

Describe the bug The pagination div in a repeater is the last item of the repeater's list. This is wrong, the pagination SHOULD NOT be part of items list. This should be obvious by design, but the real issue is that it messes with CSS rules when applying to repeater's list or items. For instance, using flexbox properties or grid affects the pagination div and destroys layout.

Have you tried all the steps at https://oxygenbuilder.com/documentation/troubleshooting/troubleshooting-guide/? Yes

Are you able to replicate the issue on a Sandbox install at https://oxygenbuilder.com/try? https://oxygen-qn52kdaf76v8k.oxygen-demo.qsandbox.me/sample-page/

To Reproduce Steps to reproduce the behavior:

  1. Go to any page
  2. Add a repeater with a any post query (at least one post needed to check the result)
  3. Apply 2 or 3 columns grid to repeater or flexbox 50% width and flex-wrap
  4. See that pagination is in a column in the last row instead of at bottom of the list

Expected behavior Pagination div should be at the bottom of the list, AFTER the list, out ouf the .oxy-dynamic-list div.

What actually happens Pagination div is inside the list, so it is part of the columns!

Screenshots repeater pagination

Desktop (please complete the following information):

KittenCodes commented 4 years ago

You can move the pagination below the repeater with CSS:

.oxy-repeater-pages-wrap {
    width: 100%;
}
yankiara commented 4 years ago

OK, but semantically speaking, pagination should not be a element of the repeater's list. What about dynamically ordering the list or any other process in JS? The pagination div parasites all CSS or jQuery find.

d-packs commented 4 years ago

You can move the pagination below the repeater with CSS:

.oxy-repeater-pages-wrap {
  width: 100%;
}

That's not really a solution and this problem is real. Pagination should definitely not be treated like any other item in the list. It's a concept flaw really.

rickfitz commented 4 years ago

This completely messes up putting the repeater in a carousel like Flickity. There's always one extra slide that shouldn't be there. So looks like I now have to add extra js to delete the last element every time :(

roireshef commented 4 years ago

Guys (@KittenCodes) - any clean solution?

KittenCodes commented 4 years ago

Guys (@KittenCodes) - any clean solution?

Only the above CSS.

yankiara commented 4 years ago

Sometimes, there's no other choice than removing it from the DOM, for instance:

jQuery('.your-slider .oxy-repeater-pages-wrap').remove();
warm--tape commented 4 years ago

Definitely a major issue. Tried applying CSS grid to a repeater. Should be simple enough, but no- layout ruined because pagination becomes one of the grid items and adds extra spacing at the bottom of the grid.

Screen Shot 2020-07-17 at 4 44 23 PM Screen Shot 2020-07-17 at 4 44 08 PM
KittyFolks commented 4 years ago

Is this going to be addressed soon? I have tons of repeaters all across my site and there is no chance I switch them all to manual to add "no_found_rows" manually and rewrite the whole query. The whole point of using a repeater is lost. The issue is that I have defined a max number of posts, which should remove the pagination automatically. As I understand, the pagination itself is added as another element and thats why the pagination shows up. I have hidden the element through CSS but this doesn't remove it from the DOM of course. There should be an option to disable pagination in custom mode.

Is there a function I can use to accomplish the removal of .oxy-repeater-pages-wrap until this is fixed? (Globally for all oxy-repeaters)

swinggraphics commented 3 years ago

Is this going to be addressed soon?

No. See the conversations about duplicate IDs and other bad coding practices. Once they release something, they stick with it.

lucianahanan commented 3 years ago

this would be so easy to fix, just by adding a counter variable after the id name.. like...

for ( $i=1; $i < $list_size; $i++ ) { print "<article id="$divname_$i"; ( ... ) }

I don't know why this was not added to priority yet....

roireshef commented 3 years ago

I honestly didn't expect them to ignore this simple request and to not provide any clean solution for so much time, so I'm going to share the 10-sec solution I came across digging into their code, specifically into wp-content/plugins/oxygen/component-framework/components/classes/dynamic-list.class.php just comment out this whole code block and this damn repeater pages element will be gone from your life:

                     ?>
                     <div class="oxy-repeater-pages-wrap">
                         <div class="oxy-repeater-pages">
                         <?php
                         $pagination_args = array(
                             'base' => str_replace( $big, '%#%', esc_url( get_pagenum_link( $big ) ) ),
                             'format' => '?paged=%#%',
                             'current' => max( 1, get_query_var('paged') ),
                             'total' => $query->max_num_pages,

                         );

                         if(isset($options['paginate_mid_size']) && is_numeric($options['paginate_mid_size'])) {
                             $pagination_args['mid_size'] =  intval($options['paginate_mid_size']);
                         }

                         if(isset($options['paginate_end_size']) && is_numeric($options['paginate_end_size'])) {
                             $pagination_args['end_size'] =  intval($options['paginate_end_size']);
                         }

                         if(isset($options['paginate_prev_link_text']) && $options['paginate_prev_link_text']) {
                             $pagination_args['prev_text'] =  sanitize_text_field(oxygen_vsb_base64_decode($options['paginate_prev_link_text']));
                         }
                         if(isset($options['paginate_next_link_text']) && $options['paginate_next_link_text']) {
                             $pagination_args['next_text'] =  sanitize_text_field(oxygen_vsb_base64_decode($options['paginate_next_link_text']));
                         }

                         echo paginate_links( $pagination_args );
                        ?>
                         </div>
                     </div>
                    <?php

I must say, not coming from the web-development domain, I can't even call reasonable covering it up with CSS to hide a redundant element, or use JS code to remove it. It shouldn't be created in the first place. It's sad those are the practices used today by so many developers.

wpsumo commented 3 years ago

this would be so easy to fix, just by adding a counter variable after the id name.. like...

for ( $i=1; $i < $list_size; $i++ ) { print "<article id="$divname_$i"; ( ... ) }

I don't know why this was not added to priority yet....

@YammYcoding I agree, or just move everything to classes. And solve the issue where we are not forced to put styling in the global universal.css stylesheet as soon as we use classes.

Have you tried an override of ID styling with your counter variable? Or was it just a quick draft? If so experienced any issues with it?

jamescrabb commented 3 years ago

And whilst they are at it they can fix the duplicated id issue with repeaters!

stefanullinger commented 3 years ago

Hi guys, so I was just running in the same issue. Actually I switched from Elementor to Oxygen because 1) Elementor has many issues and 2) many people recommended to try Oxygen. But now that I see that Oxygen also has so many issues, I will definitely search for a better solution.

For now, my quick fix for this issue with my 3-column grid is:

.oxy-repeater-pages-wrap {
  grid-column: span 3;
}
yankiara commented 3 years ago

And for a responsive grid (either with breakpoint or auto-fit), this is working:

.oxy-repeater-pages-wrap {
  grid-column: 1 / -1;
}
JoeKns commented 3 years ago

This is still an issue in May 2021. I honestly don't understand why.

d-packs commented 3 years ago

I think it's because once released, changing the HTML output of a component would break sites that are using that component. If it was horrible code upon release, than that's what we're stuck with. So your best option is to either catch the shortcode output via do_shortcode filter, or javascript to move the pagination element out of the container.

swinggraphics commented 3 years ago

It'd be easy enough keep the existing code for existing elements and use corrected code for new instances. They just don't care much about standards and best practices. If they did, stuff like this would never make it out the door to begin with. I've been admonished for making declarations like that before, but it is evidenced in every corner of their product.

v-marinkov commented 3 years ago

This must be fixed. Yet another simple issue completely ignored

KonradDRAST commented 3 years ago

I believe there won't be any changes until Oxy 4.0 (then braking changes are expected).

paulmatunog commented 3 years ago

Came here because of this issue. The code below works for me.

.oxy-repeater-pages-wrap {
  grid-column: 1 / -1;
}
bocohostwill commented 2 years ago

Came here because of this issue. The code below works for me.

.oxy-repeater-pages-wrap {
  grid-column: 1 / -1;
}

This worked for me as well

JoeKns commented 2 years ago

It's simply a joke this is actually still a thing after all this time.

JoeKns commented 2 years ago

I believe there won't be any changes until Oxy 4.0 (then braking changes are expected).

Wrong. As sad as that is, it's not a major update. We got a few minor fixes and a few minor UI improvements.

MattHag commented 2 years ago

Yes, this should have been tackled with 4.0. I really don't get why this is not handled with 4.0, which is the best update to introduce a "breaking" change. Seems like they go strictly by votes and therefore just neglect necessary stuff.

reachingmike commented 1 year ago

+1

drakedeatonuk commented 1 year ago

+1

Stoxis commented 10 months ago

Why hasn't this been added...

4tcmedia commented 2 weeks ago

This actually still not added - 2024 now This worked for me with auto fit columns in grid (4 rows) .oxy-repeater-pages-wrap { grid-column: 1 / -1; }