CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

WIP: Remove spaces and new lines from column render

Open pxpm opened this issue 3 years ago • 5 comments

WHY

BEFORE - What was wrong? What was happening before this PR?

reported here: https://github.com/Laravel-Backpack/CRUD/issues/4270

Backpack columns are pieces of html when given to the datatables to render. They include the formating your wrote on the blade file like \n and \r etc so it will be presented in the "page source-code" as you wrote them.

When displaying in browser it's fine because browser know how to interpret it. Trying to export that is not so funny! 🙃

Thing is, in datatables columns we don't need the "source code formated", we just need everything to be there, even if in one single line.

AFTER - What is happening after this PR?

We strip \n\r and eveything than's more than one white space from the returned html.

The only downside I found out until now is that something.......like....this will be transformed into: something.like.this (dots represents white spaces), anyway does anybody know any use case for double spaces ??

HOW

How did you achieve that, in technical terms?

transformed the returned html from view

Is it a breaking change?

I hope not

pxpm avatar Jul 22 '22 12:07 pxpm

@tabacitu I need your opinion on this before I commit any more time here. There is also this comment: https://github.com/Laravel-Backpack/CRUD/issues/4270#issuecomment-1192525886

pxpm avatar Jul 22 '22 12:07 pxpm

I think this is freaking awesome @pxpm . Is it really so small, the change? Hell yeah! 🎉 Why do you have doubts - is there a downside I'm not seeing?

Devil's advocate... would anybody WANT the newlines on... Show, for example? 🤷‍♂️ I don't think so... right? Wait, how about custom_html... or like a big textarea in the Show operation? Wouldn't you want newline characters there? 👀

tabacitu avatar Jul 22 '22 12:07 tabacitu

@tabacitu I don't think it has any influence in show because we include the view directly without using that function. That function is only used for datatables columns in getEntriesAsJsonForDatatables() in Search trait.

pxpm avatar Jul 22 '22 14:07 pxpm

Damn! Good thinking @pxpm . Sounds like a really good solution then. Yes, let's do it.

What else does this need, to be merged?

tabacitu avatar Jul 23 '22 11:07 tabacitu

@tabacitu I have only one concern that I found and mentioned earlier, I think it's no biggie, but I need approval to have peace of mind:

The only downside I found out until now is that "something.......like....this" will be transformed into "something.like.this" (dots represents white spaces)

There is also the solution to have all the field definition in one line, it feels safer to me because we don't need to str_replace the white lines, but the field will become .... something unusual!! 🙃 I reported it here: https://github.com/Laravel-Backpack/CRUD/issues/4270#issuecomment-1192525886

I have not the safe in mind to tell you merge this without us brinstorming abit about the solutions.

Cheers

pxpm avatar Jul 25 '22 08:07 pxpm

Ping! I'm hoping https://github.com/Laravel-Backpack/CRUD/pull/4659 will provide an alternative solution to this, JS instead of PHP. Check it out please 🙏

tabacitu avatar Sep 13 '22 05:09 tabacitu