wire-elements / modal

Livewire component that provides you with a modal that supports multiple child modals while maintaining state.
MIT License
1.12k stars 131 forks source link

Making max-width responsive #26

Closed roni-estein closed 2 years ago

roni-estein commented 3 years ago

Due to the sm:w-full in the modal's body from tailwind ui, you need a way to control the max-width or it will flicker out at smaller breakpoints if your size is too large.

Size is being set here as only one class, but since it's css there is an easy win that doesn't require a huge programmatic change.

https://github.com/livewire-ui/modal/blob/112b521bbd9eef4c188b9eaa0b274dbe424b6578/resources/js/modal.js#L37

This can be modified to instead of just pushing in that one value, I'd use a look up table and pull it from there: here is a working one I tested.

public static array $sizes = [
        'sm' => 'sm:max-w-sm',
        'md' => 'sm:max-w-md',
        'lg' => 'sm:max-w-md md:max-w-lg',
        'xl' => 'sm:max-w-md md:max-w-xl',
        '2xl' => 'sm:max-w-md md:max-w-xl lg:max-w-2xl',
        '3xl' => 'sm:max-w-md md:max-w-xl lg:max-w-3xl',
        '4xl' => 'sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-4xl',
        '5xl' => 'sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-5xl',
        '6xl' => 'sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-5xl 2xl:max-w-6xl',
        '7xl' => 'sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-5xl 2xl:max-w-7xl',
    ]; 

I used it in my own modal system, I was thinking of pulling in this one to solve my child model issue but there are two outstanding tickets that i need solved first. Luckily they both have simple fixes :)

Hope this helps! I'm excited to start using this!

gcw07 commented 3 years ago

This and the current implementation really ties it to Tailwind. I love Tailwind and use it in majority of my projects, but it seems like this shouldn't be so tightly integrated when it doesn't have to be. It is one thing for the default to be Tailwind specific, but it should be able to be used with other CSS frameworks.

The solution seems to be that instead of setting just a size 'sm', 'md', 'lg', etc., that a CSS class just be set. So you could just pass in 'sm:max-w-sm' or 'sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-5xl 2xl:max-w-7xl' and have that class variable merged just like it is now into the template. This would allow this package to be used with other CSS frameworks as the developer would just need to publish the template and change the classes, but then set the size via a style related to that framework.

roni-estein commented 3 years ago

Grant, you bring up a valid point, but this issue is narrowly focused on responsive widths. This entire package is packed with tailwind and will completely implode without it. Unless there was an alternative.

What you are suggesting a complete rebuild. To some arbitrary css maybe bootstrap but perhaps with publishable assets. And perhaps a command to choose between some presets. Which sounds great. But it seems like that should be a different issue.

gcw07 commented 3 years ago

Most of the Tailwind code could be more generalized without any rebuild. The most difficult solution would be changing these lines:

https://github.com/livewire-ui/modal/blob/main/resources/js/modal.js#L69 https://github.com/livewire-ui/modal/blob/main/resources/js/modal.js#L71

The rest of Tailwind code is in the blade file that is publishable, so easily changable by anybody and then these

https://github.com/livewire-ui/modal/blob/main/resources/js/modal.js#L30 https://github.com/livewire-ui/modal/blob/main/resources/js/modal.js#L37

Both of those could be switched to a general "maxWidthClass" or something. Why I added it to this thread, is since it was related to changing the max width anyways. But all of this would be up to @PhiloNL anyways. Just an idea to make it easier to use without having to fork the entire project to use it with another CSS framework.

roni-estein commented 3 years ago

Ahh I see your point, yes that CSS is simply the fix for the visual issue with the CSS in its current state, but if everything is publishable and configurable, it makes sense that this should be as well. Thats a valid point. It really doesn't matter where it's stored as long as there is a way to set a key and what is put there as a result of the key.

The reason the it's even needed is when the internal items in the modal naturally push the drawing outside of the width of the screen and not the max-width of the modal.

I only exposed the bug because I had a design that required a table in a specific modal and was surprised when people with specific devices mentioned no modal showed up. but the screen overlay, the bg-gray-500 bg-opacity-75.

So this solution proposed is simply to deal with that specific issue that has cropped up for a few people using the tailwind modal as is when we are parameterizing the size.

PhiloNL commented 3 years ago

Thanks @roni-estein I will take a look at this, somewhere this week 😄 It's taking a bit more time than usual.

@gcw07 @roni-estein I will take a look at generating a scoped Tailwind CSS file that can be used when working with other CSS frameworks. Thanks for the feedback.

roni-estein commented 3 years ago

Just had a few seconds between tasks, and I really to delegate my modals to this package, here is a quick illustration of the breakpoint issue. With the default install of livewire and tailwind 2.1+.

Component

<?php

namespace App\Http\Livewire\Modals;

use App\Models\Review;
use LivewireUI\Modal\ModalComponent;

class DeleteReview extends ModalComponent
{
    public ?Review $review;

    public function mount(Review $review)
    {
        $this->review = $review;
    }

    public function delete()
    {
        $this->review->delete();

        $this->forceClose()->closeModal();
    }

    public function cancel()
    {
        $this->closeModal();
    }

    public function render()
    {
        return view('livewire.modals.delete-review');
    }
}

Blade View (literally cut and pasted from tailwindu, I didn't set up events on the buttons yet but just to show the maxwidth issue)

<div class="bg-white px-4 pt-5 pb-4 sm:p-6 sm:pb-4">
    <div class="sm:flex sm:items-start">
      <div
        class="mx-auto flex-shrink-0 flex items-center justify-center h-12 w-12 rounded-full bg-red-100 sm:mx-0 sm:h-10 sm:w-10">
        <!-- Heroicon name: outline/exclamation -->
        <svg class="h-6 w-6 text-red-600" xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24"
             stroke="currentColor" aria-hidden="true">
          <path stroke-linecap="round" stroke-linejoin="round" stroke-width="2"
                d="M12 9v2m0 4h.01m-6.938 4h13.856c1.54 0 2.502-1.667 1.732-3L13.732 4c-.77-1.333-2.694-1.333-3.464 0L3.34 16c-.77 1.333.192 3 1.732 3z"/>
        </svg>
      </div>
      <div class="mt-3 text-center sm:mt-0 sm:ml-4 sm:text-left">
        <h3 class="text-lg leading-6 font-medium text-gray-900" id="modal-title">
          Deactivate account
        </h3>
        <div class="mt-2">
          <p class="text-sm text-gray-500">
            Are you sure you want to deactivate your account? All of your data will be permanently removed. This action
            cannot be undone.
          </p>
        </div>
      </div>
    </div>
  </div>
  <div class="bg-gray-50 px-4 py-3 sm:px-6 sm:flex sm:flex-row-reverse">
    <button type="button"
            class="w-full inline-flex justify-center rounded-md border border-transparent shadow-sm px-4 py-2 bg-red-600 text-base font-medium text-white hover:bg-red-700 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-red-500 sm:ml-3 sm:w-auto sm:text-sm">
      Deactivate
    </button>
    <button type="button"
            class="mt-3 w-full inline-flex justify-center rounded-md border border-gray-300 shadow-sm px-4 py-2 bg-white text-base font-medium text-gray-700 hover:bg-gray-50 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-indigo-500 sm:mt-0 sm:ml-3 sm:w-auto sm:text-sm">
      Cancel
    </button>
  </div>

This is it in action.

https://user-images.githubusercontent.com/8517475/117342752-30e44f00-ae69-11eb-8e61-cf5234a1e381.mov

jon-groovy commented 3 years ago

a bit hackey but you can use modalMaxWidth inside your component to "fix" this for now.

Using 5xl from @roni-estein's lookup table as an example, the code below would append the following classes: sm:max-w-md md:max-w-xl lg:max-w-3xl xl:max-w-5xl

public static function modalMaxWidth(): string
{
    return 'md md:max-w-xl lg:max-w-3xl xl:max-w-5xl';
}

this would obviously break with any update.

roni-estein commented 3 years ago

Just wanted to illustrate it, so show what I meant and how prevalent and sneaky it is.