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

NEW SearchableDropdownField #11057

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Nov 20, 2023

Issue silverstripe/silverstripe-admin#1618

This PR is derived from silverstripe/tagfield

Note that it differs from tagfield in that it uses "ID" (int) for "Value" rather than "Title" (string) which was fine for simply creating tags, but not appropriate for regular relations. This means this field cannot be used for creating new objects like tagfield can, so it's not a full replacement for tagfield.

Admin PR

For method signatures that don't match parent classes e.g. SingleSelectField it's because they now either have strong return types, or looser params defined in docblocks. This is allowed in PHP via covariance (tighter return types on sublasses) and contravariance (looser param types on subclasses) - https://www.php.net/manual/en/language.oop5.variance.php

@GuySartorelli
Copy link
Member

I've only tested this using the "only these users" field so far, but in that scenario the lazy load limit doesn't seem to be applying - I have a lot more than just 10 items displaying.

@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch 2 times, most recently from 324b4e9 to 1adb5ab Compare December 12, 2023 20:58
@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch 2 times, most recently from f2acbac to 84f40ed Compare December 12, 2023 22:08
@GuySartorelli
Copy link
Member

The problem mentioned in #11057 (comment) has still not been fixed

@GuySartorelli
Copy link
Member

GuySartorelli commented Dec 13, 2023

Not sure if this is an admin or a framework PR problem, but with a many_many list, the values aren't saved (or, if they are saved, they at least don't display in the field after saving)

Code:

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\SearchableMultiDropdownField;
use SilverStripe\Taxonomy\TaxonomyTerm;

class Page extends SiteTree
{
    private static $many_many = [
        'Terms' => TaxonomyTerm::class
    ];

    public function getCMSFields()
    {
        $fields = parent::getCMSFields();
        $fields->addFieldsToTab('Root.Main', [
            SearchableMultiDropdownField::create('Terms', 'Terms', TaxonomyTerm::get(), $this->Terms(), 'Name'),
        ]);
        return $fields;
    }
}

Update: The same thing happens for a has_many relation - doesn't save the values, or if they are saved they at least aren't displaying in the field.

@GuySartorelli
Copy link
Member

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected:
image
(note that "a" here is the name of a taxonomy term in the db)

The has_one relation does correctly save when I select a different option and save the page, so that's good.

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 13, 2023

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected:

That's the default behaviour of the existing Dropdownfield and I replicated it. It's a really obnoxious default behaviour IMO though can be switched off by just called ->setHasEmptyDefault(true);

@emteknetnz
Copy link
Member Author

emteknetnz commented Dec 13, 2023

the values aren't saved

Your code example worked fine on my computer, the values saved, did you forget to dev build flush?

@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch from 84f40ed to 333a74a Compare December 13, 2023 02:52
@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch from 333a74a to 8c89956 Compare December 13, 2023 21:03
@GuySartorelli
Copy link
Member

the values aren't saved
Your code example worked fine on my computer, the values saved, did you forget to dev build flush?

I did not forget - though clearly either I did something wrong, or the latest batches of changes fixed it, because it works now 👍

@GuySartorelli
Copy link
Member

With a has_one relation, before trying to save any records, the first option is displayed as though it's selected:
That's the default behaviour of the existing Dropdownfield and I replicated it. It's a really obnoxious default behaviour IMO though can be switched off by just called ->setHasEmptyDefault(true);

That's not the expected behaviour in my mind. Just because DropdownField does that doesn't mean this field should. DropdownField isn't explicitly for working with relations. This field is explicitly for working with relations - and as such the expected behaviour changes a little. It should show that there is no relation stored in the field until such a time as the user stores a relation in the field.

My recommendation is to call $this->setHasEmptyDefault(true); in the constructor. This will preserve the functionality to allow the first item as a default value if people really want that, but requires developers to opt into that.

@GuySartorelli
Copy link
Member

This still needs to be addressed: #11057 (comment)

@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch from 8c89956 to 4772861 Compare December 13, 2023 22:39
@emteknetnz
Copy link
Member Author

Have added the $this->setHasEmptyDefault(true) on the single dropdown contructor

Have rebased to get https://github.com/silverstripe/silverstripe-framework/pull/11073/files which solves the lazyLoad limit not being respected

@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch 3 times, most recently from a71d2cd to 17a3acc Compare December 13, 2023 23:25
@GuySartorelli
Copy link
Member

As discussed in person, please set the default lazy-load limit to 100

@emteknetnz emteknetnz force-pushed the pulls/5/selectorfield branch from 17a3acc to 23eca53 Compare December 14, 2023 02:28
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, works great locally.

@GuySartorelli GuySartorelli merged commit c903207 into silverstripe:5 Dec 14, 2023
15 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/selectorfield branch December 14, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants