Clay: Add iframe onload handler to ClayModal API

[markocikos]

Proposal:

<ClayModal
    onIframeLoad={doSomething}
>
</ClayModal>

or

const {observer} = useModal({
    onIframeLoad={doSomething},
})

I'm working on Update Modal Patterns to match Lexicon Definition. I have now encountered two cases where this kind of a handler would be useful:

1. We need to add a loading indicator to modal. This loading indicator would start when ClayModal containing iframe is rendered, and stop when iframe content is loaded.
2. Currently, ClayModal hardcodes iframe height to 160px, see here. This cuts off modal content, for example in fragments-web. Ideally, we would want height to have the same size as content. I don't think there is a CSS way to achieve this. It can be done with JS, but we need to hook onto this load event.

Alternatives:

1. Maybe there is a way to reference iframe from outside component? A DOM query after render to bind a listener would be a race condition and ugly overall. ClayModal exposing iframe ref also seems bad.
2. Maybe there is a CSS way to achieve autoheight?

/cc @jbalsas

[jbalsas]

Hey @laugardie, @drakonux, could you take a look at this from a UX perspective? What would be the desired default behaviour?

I think @marcoscv-work and @eduardoallegrini worked on some CSS solution for the Modals heigh/scroll... can you guys dig that up?

IMHO... a height-changing modal is a terrible experience. Unless done with a "cool" transition (which soon gets repetitive and unnecessary...), flickering elements give systems a clunky and slow feeling. One of the "best" improvements we added in 7.2 was to make sure that css classes in iframes were properly set up at the beginning to avoid FOUC. Modals will take the same amount of time to load, but by avoiding the flickering, it seemed way faster.

So... I'd suggest to stick with some fixed heights (might be configurable) and make the content scrollable.

Just my 2 cents :)

[jbalsas]

Ah, another thing... it feels like modals with iframe should always have the loading indicator... so that might be something worth baking in even if we also provide an external API to hook into the load event.

[markocikos]

Maybe we could extend Lexicon size recommendation to include not only width, but height as well. To have all modals in 16:9 ratio for example. If the content height is too big, you could choose a bigger size, topping up with full screen.

Fixed ratio also has additional benefit of making it possible to have responsive iframe with only CSS, see for example here.

[laugardie]

As @jbalsas said, height-changing modal when loading content is not a good experience and we should avoid that transition. We do recommend having as default, a full-screen size for iframes so that the content would be loaded on the screen avoiding that jumping effect.

[markocikos]

We do recommend having as default, a full-screen size for iframes

@laugardie Sounds good! I will add that default in the code. This should be also added to iframe section of modals docs.

This resolves the size issue. This proposal in this ticket still makes sense for the loading indicator.

[bryceosterhaus]

it feels like modals with iframe should always have the loading indicator

Yeah this seems like a good idea. I think we can also provide an iframeProps prop for modal so that users can also add any other params or behaviors as needed to that element.

[kresimir-coko]

Hey @bryceosterhaus, I've spent some time familiarising myself with the Modal component before i tackle this on Monday. So I've got one question about it, should I use useEffect or do you know from the top of your head if useModal could be leveraged for this use case.

[matuzalemsteles]

hey @bryceosterhaus and @kresimir-coko I think this should not be in the useModal hook nor in the ClayModal main component, who takes care of rendering the iframe content is <ClayModal.Body>, I think we just pass ...otherProps to the element would be enough here to cover this case and other properties that they wish to pass.

[bryceosterhaus]

Yeah I agree with @matuzalemsteles. I think right now, it makes the most sense to add ...otherProps or specifically iFrameProps to the <ClayModal.Body> component. So no need to add anything within the hooks.

[bryceosterhaus]

added in https://github.com/liferay/clay/pull/3139