Kreyu / data-table-bundle

Streamlines creation process of the data tables in Symfony applications. NOT PRODUCTION READY.
https://data-table-bundle.swroblewski.pl
MIT License
69 stars 15 forks source link

LinkColumnType : why default target="_self" #103

Closed bdecarne closed 2 months ago

bdecarne commented 2 months ago

Hi !

Is there any reason to make target option required and "_self" by default in the LinkColumnType ?

I have the impression that this behavior causes Turbo Drive to malfunction. I'm not a specialist in this library, but I get the impression that links with target=_self are ignored by Turbo.

EDIT : i just saw this issue https://github.com/hotwired/turbo/issues/1113

bdecarne commented 2 months ago

To complete my message :

I think the <turbo-frame> around the table should have the property target="_top" to force column or action links to navigate outside the frame. Then, pagination links should have the property data-turbo-frame="_self" to force them to render into the frame.

I can make the MR if it makes sense

Kreyu commented 2 months ago

Hey @bdecarne! Thanks for the report. I quickly glanced at one of my applications using the bundle, and I can confirm you're right.

Is there any reason to make target option required and "_self" by default in the LinkColumnType ?

To be honest, I simply looked at the docs of the anchor element, which described a _self is used by default. I thought that explicitly setting this attribute would change nothing. I'm up for making this option nullable by default.

So, as you suggested, this would require making following changes:

I've noticed the action_button_value is exactly the same as action_link_value - saving this one for the small refactor of themes (#91).

Pull request would be highly appreciated, if you're willing to do that!

Cheers.

Kreyu commented 2 months ago

Upon further testing, I don't really understand what difference would the data-turbo-frame="_self" make. Is it really required? If so, I think it should also be applied on sortable column headers. But when I'm testing this on one of my apps, pagination button with and without this attribute work the same (or maybe I am missing something here)

bdecarne commented 2 months ago

You are right : sortable links should have this property too.

It works without, but the whole page is loaded instead of just the frame. It makes the <turbo-frame> tag useless, i guess.

Kreyu commented 2 months ago

Alright, thanks for confirming. I was focused solely on the fact that the request was sent asynchronously, not that it targeted the whole page, instead of a frame.

Kreyu commented 2 months ago

Thanks for the help! Included in the v0.18.1 release.