Updates to remove flashing/disappearing modal fixes issues 26

[roni-estein]

I tried to do this is in the least damaging way possible that also allows for classes to be easily changed to suit the needs of the project. By limiting the modal size to not over run the screen, the modal will always be able to render on the viewport.

This will break for people who have published their classes, so I've included a list of the changes I've made that they can adapt. You Can also just look at the changed filed in the PR.

Only do these if you published or overrode the classes from laravel-ui/modal

1. Add 'maxWidthClass' => $componentClass::modalMaxWidthClass(), to the 'modelAttributes' in Modal Class
2. Add the protected static array $maxWidths to the abstract ModelComponent
3. copy the static function modalMaxWidthClass to abstract ModelComponent
4. In resources/js/modal.js, set the default class to null
5. In the same file replace this line this.modalWidth = 'sm:max-w-' + this.getActiveComponentModalAttribute('maxWidth'); with this one this.modalWidth = this.getActiveComponentModalAttribute('maxWidthClass');twice.
6. Finally in the same file initialize the the default setting of maxWidth to your configured default in this way...

in init() {

// add this line
this.modalWidth = this.getActiveComponentModalAttribute('maxWidthClass');

// before
this.$watch...

7. run npm run prod and you should publish a new modal.js that will now reflect those changes.

*** Remember that only if you have published some or all of your classes and configs otherwise this should just work ***

PR'd with ❤️
-Roni

[PhiloNL]

Perfect! ❤️
Thanks for the PR @roni-estein

[PhiloNL]

@roni-estein I think I found an issue (although I merged the PR I haven't created a release yet). See the comments. Could you verify? 😄

[PhiloNL]

For me, it doesn't work unless I change this line into:

$componentClass::modalMaxWidthClass($componentClass::modalMaxWidth()),

Otherwise, it will always be '2xl', is that correct?

[roni-estein]

right !

[roni-estein]

I was running the tests by changing the configs visually! I couldn't find a good way to dusk test this, but it lead me to miss this which kind of was the main intent of not locking it down! thats embarassing! 😬😳

[PhiloNL]

No worries! 🙌

[PhiloNL]

Alternatively, maybe change this?

    public static function modalMaxWidthClass(): string
    {
        $maxWidth = static::modalMaxWidth();
        
        if(array_key_exists($maxWidth, static::$maxWidths)){
            return static::$maxWidths[$maxWidth];
        }
        
        return static::$maxWidths[static::modalMaxWidth()] ;
    }

[roni-estein]

That exposes a super subtle issue, there is no real validation for this, it's not input. I was coding it in my head, but my mental model was a tiny bit off because I'm always coding for the user and not the "coder"! We are the ones calling it so we basically need to show someone where they made an illegal entry and where to fix it! We really don't need the guard clause or defaults anymore, we are not trying to change their code, just show them what to change.

Since we have limited them to a predefined list of keys from sm -> 7xl we should simply throw an error with a solid explanation of the issue and what to change or override to make it work

Another thing that could be done is make the array itself a config option.

I'll do that in a separate PR if you like that idea. It won't break anything.

Here is my idea a bit rough.

Leave the rest of the code as is but change this function to this as you recommended,

public static function modalMaxWidthClass(): string
{
    if ( ! array_key_exists(static::modalMaxWidth(), static::$maxWidths) ) {
        
        throw new \Exception('Illegal key ' . static::modalMaxWidth() . ' not found in available protected static $maxWidths array in LivewireUI\Modal\ModalComponent. Either extend the class with your own array or choose another modalMaxWidth(). Please refer to https://github.com/wire-elements/modal/issues/26');
    }
    
    return static::$maxWidths[ static::modalMaxWidth() ];
}

[PhiloNL]

Thanks for the suggestion, I've made the changes in #113 😄