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

DependsOnNot bug fix with strict check #184

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

xUJYx
Copy link

@xUJYx xUJYx commented Jul 19, 2021

For 'dependsOnNot' - changed 'strict check' to 'check with coercion' cause all form values are always strings.
Error occurs when checking for integer values and they comparing with string equivalents from html form.

@AlbinoDrought
Copy link

AlbinoDrought commented Jan 31, 2022

Ran into this issue with the unforked epartment/nova-dependency-container as well, here's a quick repro:

image

<?php

namespace App\Nova\Actions;

use Epartment\NovaDependencyContainer\NovaDependencyContainer;
use Laravel\Nova\Actions\Action;
use Laravel\Nova\Fields\Select;
use Laravel\Nova\Fields\Text;

class SampleDependsOnNot44 extends Action
{
    const SOME_OPTION_A = 1;
    const SOME_OPTION_B = 2;
    const SOME_OPTION_C = 3;

    public function fields()
    {
        return [
            Select::make('Option', 'option')
                ->options([
                    self::SOME_OPTION_A => 'A, show dependency field',
                    self::SOME_OPTION_B => 'B, hide dependency field',
                    self::SOME_OPTION_C => 'C, show dependency field',
                ]),

            NovaDependencyContainer::make([
                Text::make('Hide me for option B'),
            ])->dependsOnNot('option', self::SOME_OPTION_B),
        ];
    }
}

Expected result:

image

@xUJYx
Copy link
Author

xUJYx commented Jan 31, 2022

Expected result:

image

Have you used code from my fork? Or from epartment/nova-dependency-container itself?

@AlbinoDrought
Copy link

Have you used code from my fork? Or from epartment/nova-dependency-container itself?

Sorry, just from epartment/nova-dependency-container. I'll update my above comment

@xUJYx
Copy link
Author

xUJYx commented Feb 1, 2022

Have you used code from my fork? Or from epartment/nova-dependency-container itself?

Sorry, just from epartment/nova-dependency-container. I'll update my above comment

That functionality wasnt merged into main repo. So this functionality doesnt work on original epartment/nova-dependency-container package...

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.

2 participants