bitcrowd / bitstyles_phoenix

A collection of Elixir phoenix helpers for bitstyles
ISC License
12 stars 0 forks source link

Add component: modal #16

Closed planktonic closed 1 year ago

planktonic commented 3 years ago

Should include the button to open the modal, and the modal itself. As with the dropdown menu in #15 we should include the JS to actually make it work (alpinejs)

How to handle the modal’s close button in each of the cases?

See https://bitcrowd.github.io/bitstyles/?path=/docs/ui-modal--modal

maltoe commented 2 years ago

@planktonic @andreasknoepfle recently built this in a project:

      # TODO: implement in BitstylesPhoenix
      # TODO: ask Darren about south/east borders
      # TODO: add transitions https://youtu.be/Of1phFsC4ZI?t=2555
      # TODO: originally it had window-keydown, but that makes it instantly go away when the user does *anything*?!
      # TODO: get rid :focus underline under X button
      ~H"""
      <div class="o-modal" aria-labelledby={assigns[:title] || gettext("Modal")}>
        <div
          phx-click={JS.dispatch("click", to: "#modal-close")}
          class="o-modal__overlay"
        >
          <%= live_patch "✖", to: @return_to, id: "modal-close", tabindex: "-1", title: gettext("Close modal") %>
        </div>
        <div class="a-content a-content--m o-modal__content a-card u-padding-xl@m u-padding-l u-flex u-flex-col" role="document">
          <%= render_slot(@inner_block) %>
        </div>
      </div>
     """

update

latest version

      ~H"""
      <div class="o-modal" aria-labelledby={assigns[:title] || gettext("Modal")}>
        <div
          phx-click={JS.dispatch("click", to: "#modal-close")}
          class="o-modal__overlay"
        >
          <%= live_patch to: @return_to, id: "modal-close", tabindex: "-1", title: gettext("Close modal") do %>
            <%= ui_bitstyles_icon("cross", focusable: false, aria_hidden: true) %>
            <span class="u-sr-only"><%= gettext("Close modal") %></span>
          <% end %>
        </div>
        <div class="a-content a-content--m o-modal__content a-card u-padding-xl@m u-padding-l u-flex u-flex-col" role="document">
          <%= render_slot(@inner_block) %>
        </div>
      </div>
planktonic commented 2 years ago

Nice! What’s the issue with the borders?

For the close button:

Something like:

<button>
  <svg aria-hidden="true" focusable="false">…</svg>
  <span class="u-sr-only">close modal</span>
</button>

When it closes we could either remove it from the DOM or add the hidden attribute, whichever makes most sense.

Transitions already exist in the CSS, but we can rework or improve those if needed — right now there’s a fade-in combined with a little scaling and vertical transform when the modal appears, but the CSS that triggers it may not fit exactly with the implementation you’ve got here. For transition-out there are two classes .o-modal--animation-ok and .o-modal--animation-cancel that create two slightly different animations (based on whether the user clicked ok or cancel, if those buttons exist like in a dialog). Again that might need some CSS changes to work with this implementation (anyway I’d like to update the component CSS, and add some examples to the docs, which I would do based on the implementation we build here)

maltoe commented 2 years ago

When it closes we could either remove it from the DOM or add the hidden attribute, whichever makes most sense.

At least with the LiveView / push_patch solution it's going to be removed. With Alpine we would need to hide it, I guess.

maltoe commented 2 years ago

Screenshot 2021-11-29 at 09 53 16

borders: Do they look the same on your machine/browser? If so, it's basically personal taste, I'm a little disturbed by the different background around the model and the non-rounded corners right/bottom.. Overall maybe just confused what the design concept is behind it?

planktonic commented 2 years ago

Hmm, no! Do you have a mouse connected? Your screenshot looks like scrollbars overlaying the edges. For me:

Screenshot 2021-11-29 at 10 55 43
maltoe commented 2 years ago

You're right, I have an external mouse connected and if I disconnect it the border disappears. There are no scrollbars though, apparently Chrome just reserves space for them?!

Can we still change this somehow?

andreasknoepfle commented 2 years ago

Can you move this into a ticket on bitstyles, so that we don't pollute this one :) ?

planktonic commented 2 years ago

👍 https://github.com/bitcrowd/bitstyles/issues/574

maltoe commented 2 years ago

@planktonic can't get the transition to work, something is wrong with o-modal--animation-ok

can't we have a generic

          .fade-out {
              visibility: hidden;
              opacity: 0;
              transition: visibility 0s 0.2s, opacity 0.2s linear;
          }

?

andreasknoepfle commented 1 year ago

We should make the closing button be a slot and have 3 components again:

1) modal without JS and a slot for closing button (<- let's start with this) 2) Live implementation 3) JS Alpine implementation

andreasknoepfle commented 1 year ago

Using this in our projects seems to proof that this is great as is for now.

Possible additions: