CRUD icon indicating copy to clipboard operation
CRUD copied to clipboard

New column type to display a link

Open rabol opened this issue 4 years ago • 6 comments

New href column type

can be used like this:

CRUD::column('url')->type('href');

or like this

CRUD::column('url')->type('href')->text('Text to be shown instead of url');

rabol avatar Feb 07 '21 13:02 rabol

Hi @rabol, thank you for your contribution 🙌

We're now in feature freeze mode for v4.1, so I'll move this to v4.2.


There are some ways to add links to columns, we have the wrapper attribute that can be used to do this;

// This will create a column with the name wrapped in a `a` tag,
// with the href of another column `url`
CRUD::column('name')->wrapper([
    'href' => function ($crud, $column, $entry) {
        return $entry->url;
    },
]);

Anyway, your field makes it much easier, I approve it 👌 Thank you once again!

promatik avatar Feb 07 '21 23:02 promatik

The inspection completed: No new issues

scrutinizer-notifier avatar Feb 07 '21 23:02 scrutinizer-notifier

@rabol , we're soon starting working on 4.2, so this will get merged soon 🎉 Thanks again for submitting this. Nice and simple, I love it.

I'll take this opportunity to do a quick review of how I've seen people use links for their Eloquent Models, so that we make sure the new column type accounts for them all (or doesn't, but... we know that 😀):

  • store the entire URL in the DB (https://project.com/article/how-to-do-something)
  • store the slug in the DB (how-to-do-something)
  • store the URL path (without the domain) in the DB (articles/how-to-do-something)

Fortunately, in all of the cases above, there should be an accessor on the Model, that returns the full URL (https://project.com/article/how-to-do-something) - I regularly use getLinkAttribute() for that, and I've seen a lot of people do the same. Assuming this, I think the VALUE of the link is ok to be taken directly from the given column (or accessor). So I'm OK with how this is done here. But:

  • [ ] we should probably add prefix and suffix just in case people want to add stuff before/after the value;
  • [ ] I don't think in most cases, by default, the visible text should be the URL; the URL is long and it would ruin the table; plus, I don't think anybody cares to SEE the URL, they want to CLICK it; so maybe we set the default text to something like "Open"? nice and short?
  • [ ] I think a better name for the column would be link, not href;
  • [ ] I think an additional target attribute would be useful, so that devs can define if they want the link to open in _blank or something; in fact, I think we can dump ALL custom attributes on the anchor tag... maybe they want to define the class, or style, or... I don't know; this would provide maximum flexibility, I think; though if we do that... we're getting pretty close to the text column... maybe it'd be easier to just include() that one, after we set a few defaults? food for thought...

I'll let this marinate a little bit before I do the changes above 😀 Let me know what you guys think.

Cheers!

tabacitu avatar Sep 06 '21 10:09 tabacitu

I like to suggest adding rel= no referrer and no opener to the a tag by default. just so u know so if it is an external link. they wont know which is the referring site. Maybe also an option to open in a new tab or same window.

ziming avatar Apr 09 '22 16:04 ziming

@ziming I think you can do that with wrapper:

'wrapper' => [
'element' => 'a',
'rel' => 'noreferrer'
];

// for multiple without overrides you can:

public function noReferrerLink() {
    return [
        'element' => 'a',
        'rel' => 'noreferrer'
    ];
}

'wrapper' => $this->noReferrerLink(),

//OR

'wrapper' => array_merge($this->noReferrerLink(), ['class' => 'smt'])

If you need it globally you can create an helper function or add it as a macro into crud panel ?

pxpm avatar Apr 11 '22 10:04 pxpm

Ah i see thank for the tip, I just think adding noreferrer and noopener by default to all external links is the safe and secure choice. Then if the user wants to allow the external website to know, they can disable it.

Since not everyone may know about this.

https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/noreferrer

https://developer.mozilla.org/en-US/docs/Web/HTML/Link_types/noopener

ziming avatar Jun 09 '22 02:06 ziming

Was just talking to @pxpm about this one, here's our sketch:

        // Most likely, the DB column doesn't hold the exact link, it can hold the link in multiple forms:
        // Case 1 - http://google.com
        // Case 2 - google.com -> needs http://
        // Case 3 - /article/1 -> needs url() helper
        // Case 4 - how-to-do-something -> needs prefix and url() helper
        // Case 5 - /path/to/file.jpeg -> needs asset() helper

        // NEW column type
        CRUD::column('path')
                ->type('link')
                ->text('Click here')
                ->url(function($value) {
                    return url($value);
                });
                // ->download()
                // ->target('_blank')

        // EXISTING column type
        CRUD::column('path')->wrapper([
            'href' => function($crud, $column, $entry, $related_key) {
                return url($entry->path);
            },
        ])->value(function() {
            return 'Click here';
        });

        // NEW helper function
        CRUD::column('path')->link('Click here', function($value) {
            return url($value);
        });

So what we're thinking now is... what if instead of a field type... we create a link() method that you can call on ALL column types? To automatically set the href and stuff on the wrapper? 👀

tabacitu avatar Oct 17 '22 16:10 tabacitu

On second thought... to be complete... that link() helper would end up having just as many parameters as the wrapper[href] does 😅 So it wouldn't be any easier to use...

So maybe the link column was the better idea 😅 I've moved this to a project so we take another look at it - sorry it took so long @rabol 🙏

tabacitu avatar Oct 17 '22 16:10 tabacitu

Then again... no matter how hard we try... this link column isn't going to cover all the use cases... like wrapper does... it can't... we'd have to add the functionality for each thing you can set on the <a>: download, rel, class, etc... so in the end... I think it would be either frustrating to use, because as soon as you want something complex... it fails... or a pain to maintain, since every person would ask about their favorite attribute.

So in the end... let's not do this at all... I think our current syntax isn't THAT BAD. It's not good... I think we can do better...

We'll think about it some more... and if anybody else thinks of a better solution, please reply here... but right now... it seems like we shouldn't do this, sorry guys 🙏

tabacitu avatar Oct 17 '22 16:10 tabacitu

PS. Even though we've decided NOT to create a new link column for now, a new url column WILL be created, which will work similarly to what @rabol posted above. That's because we already have a url field... so now we're adding a url column, to correspond to that field. You can see it in this PR: https://github.com/Laravel-Backpack/CRUD/pull/4675/files#diff-0f94d32eae7f2ffb61f03cd99d889df26a96d16ba3f0ff30af7ae49a76f56cb8

tabacitu avatar Oct 18 '22 11:10 tabacitu