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

Added new 'clear'-scope as requested in #3952 #4744

Closed
wants to merge 6 commits into from

Conversation

robinbonnes
Copy link
Contributor

…/october/issues/3952.

Format in config_filter.yaml:

    clear:
        label: Clear filters
        type: clear

Format in config_filter.yaml:

```
    clear:
        label: Clear filters
        type: clear

```
Copy link
Contributor

@bennothommo bennothommo left a comment

Choose a reason for hiding this comment

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

@robinbonnes
Copy link
Contributor Author

@bennothommo fixed ✔️

@Samuell1
Copy link
Member

Samuell1 commented Nov 8, 2019

I think this will be better as option in config_list.yaml something like allowResetFilter: true.

And question does this resets values to default or totally "clears" filter?

@robinbonnes
Copy link
Contributor Author

Thats also an idea, however on this way you can specify where to place the clear filter button in the filter list (start, end) and what the button text should be (the label).

Now it does reset all filters to null, so it totally clears the filters. I think resetting them back to default is better?

@LukeTowers LukeTowers added this to the v1.0.461 milestone Nov 10, 2019
@Samuell1
Copy link
Member

@robinbonnes Yeah, it should reset to default because if something cant be null then you get DB errors.

modules/backend/widgets/Filter.php Outdated Show resolved Hide resolved
modules/backend/widgets/Filter.php Outdated Show resolved Hide resolved
@msimkunas
Copy link
Contributor

msimkunas commented Oct 4, 2020

I'm not sure if this PR is still being worked on, but here goes...

The Backend\Widgets\Filter class extends the WidgetBase class which uses a trait that provides the resetSession method which is supposed to reset all session data related to the current widget. Perhaps we can use this method instead of manually setting the scope values back to their defaults?

@robinbonnes
Copy link
Contributor Author

@robinbonnes please also post a screenshot of what this looks like and add a PR to the test plugin

image

Although the PR needs some adjustments... Can we reopen this?

@msimkunas
Copy link
Contributor

In my testing, this line only updates the scope container. The content of the backend list, however, remains the same.

Removing this line inverts the issue: the list contents are refreshed but the scope container no longer updates.

@msimkunas
Copy link
Contributor

msimkunas commented Jan 5, 2021

In order to circumvent the above issue, we should not abruptly return the partial update before the filter.update event has had its chance to run at the end of the onFilterUpdate method.

The following works (and is a bit cleaner):

$updateScopePartial = false;

switch($scope->type) {
    ...
    case 'clear':
        foreach ($this->getScopes() as $scope) {
            $this->setScopeValue($scope, null);
        }

        $updateScopePartial = true;
        break;
}

$params = func_get_args();

$result = $this->fireEvent('filter.update', [$params]);

if($updateScopePartial) {
    $this->prepareVars();
    $result[] = ['.control-filter' => $this->makePartial('filter_scopes')]; 
}

if ($result && is_array($result)) {
    return call_user_func_array('array_merge', $result);
}

The $updateScopePartial controls whether the scope container should be updated since we don't want to trigger partial updates on every scope request. Also, only the inner partial of the scope container is rerendered.

@robinbonnes Currently I don't have time to make a PR for this. Would you update your PR with these changes?

@msimkunas
Copy link
Contributor

msimkunas commented Jan 5, 2021

I have also noticed that there is a bug regarding scopes with dropdown options. This PR with the changes I've suggested above clears the actual filters in the session, however, the previously selected dropdown items are still selected in the backend UI. This selection is only applied when filtering the list again though.

@LukeTowers LukeTowers changed the title Added new 'clear'-scope as requested in https://github.com/octobercms… Added new 'clear'-scope as requested in #3952 Jan 5, 2021
@msimkunas
Copy link
Contributor

This PR would also benefit from an optional resetDefault: true flag which, if true, would reset the scope values back to their defaults or clear them otherwise.

If you don't have time to continue working on this PR @robinbonnes, I could give it a go soon so do let me know.

@msimkunas
Copy link
Contributor

msimkunas commented Jan 6, 2021

One more thing. The checkbox and switch scopes must be handled differently, e.g.:

foreach($this->getScopes() as $scope) {
    $value = null;

    switch($scope->type) {
        case 'checkbox':
            $value = false;
            break;
        case 'switch':
            $value = '0';
            break;
    }

    $this->setScopeValue($scope, $value);
}

Using null as a value for these scopes does not properly reset them - those scopes are reset to their default values instead which may or may not be the desired behavior. This can be controlled with a flag (see my comment above).

@robinbonnes
Copy link
Contributor Author

@msimkunas Thanks for your suggestions, really appreciate it! I've added it to the PR.

Some questions:

  • Why add $this->prepareVars();?
  • Can't a switch/checkbox be default: true?

Ofcourse you are free to work on this PR.

@msimkunas
Copy link
Contributor

@robinbonnes The prepareVars method defines the scopes which are used inside the partial.

As for the default switch/checkbox value – I believe that the user setting should be respected here and the default (if provided) should be retrieved from the scope configuration.

@msimkunas
Copy link
Contributor

msimkunas commented Jan 17, 2021

Also bear in mind that this PR needs to reset the filter values on the client side in order to be fully functional. This requires one to somehow reinitialize the JS scope widget and IIRC the existing JS widget requires significant changes for this to be possible (i.e. it needs to unbind event handlers and properly dispose of itself upon destruction).

Another approach would be to somehow detect partial updates from within the widget and clear the scopes accordingly.

@msimkunas
Copy link
Contributor

@LukeTowers The problem, I think, is that it's only one part of the solution. See my comment for more details.

@robinbonnes
Copy link
Contributor Author

Doesn't the resetSession function also reset the list settings etc.? Maybe for now setting the filters back to their defaults and refreshing the filter partial is enough to merge this? Then in the future someone can make the JS adjustements and maybe modify the resetSession function?

@octobercms octobercms deleted a comment from bennothommo Nov 26, 2021
@octobercms octobercms deleted a comment from LukeTowers Nov 26, 2021
@octobercms octobercms deleted a comment from LukeTowers Nov 26, 2021
@octobercms octobercms deleted a comment from github-actions bot Nov 26, 2021
@octobercms octobercms deleted a comment from github-actions bot Nov 26, 2021
@daftspunk daftspunk self-assigned this Nov 26, 2021
@octobercms octobercms deleted a comment from LukeTowers Mar 4, 2022
@daftspunk
Copy link
Member

Thanks for this @robinbonnes and your insights @msimkunas

This has been merged for the next release in v2.2

@daftspunk daftspunk closed this Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants