storyblok / field-plugin

Create and deploy Storyblok Field Plugin
https://www.storyblok.com/docs/plugins/field-plugins/introduction
MIT License
25 stars 3 forks source link

feat(common): full viewport height in modal mode #215

Closed johannes-lindgren closed 1 year ago

johannes-lindgren commented 1 year ago

Field plugins will be using the full height of the modal window.

Issue: EXT-1806

image

When the field plugins enters modal mode, the height is set to auto and the autoResizer is disabled.

This pull request refactors createAutoResizer. Instead of sending the messages directly in the ResizeObserver, the code is split:

Why?

In modal mode, the scroll should be visible within the field plugin, so that we can position elements at the top and bottom of the modal window. This is important for

How to test?

You don't have to review the demo app in detail, because it is only used by us developers. If something is broken and slips past the review, there're no worries.

See https://github.com/storyblok/storyfront/pull/4225. If merged, you can test on production. If not, you must check out the branch release/church from Storyfront and run it in dev mode.

Then open up the demo app here. See how the height changes automatically in non-modal mode as you click on the increment button, while in modal mode, the height is fixed:

2023-06-23_13-10-51 (1)

Then test in the sandbox, which should have corresponding changes.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ā†—ļøŽ

Name Status Preview Comments Updated (UTC)
plugin-sandbox āœ… Ready (Inspect) Visit Preview šŸ’¬ Add feedback Aug 22, 2023 0:55am
johannes-lindgren commented 1 year ago

@eunjae-lee if you could gloss over this pull request and especially focus at:

Does it seem reasonable?

Here are the changes that will be made to Storyfront: https://github.com/storyblok/storyfront/pull/4024/files

eunjae-lee commented 1 year ago

@johannes-lindgren thanks for the work!

// createPluginActions.ts
  const onHeightChange = (height: number) => {
    if (state.isModalOpen) {
      return
    }
    postToContainer(heightChangeMessage(uid, `${height}px`))
  }

...

        if (isModalOpen) {
          postToContainer(heightChangeMessage(uid, 'auto'))
        }

I feel like this two parts are a bit too magical. That might not be the behavior users would want. They may want infinitely growing iframe height. With this change, there's no way to do that. Maybe we detach this coupling, and let user set auto as height if they want. What do you think?

johannes-lindgren commented 1 year ago

@eunjae-lee how would we decouple it? How would the user set auto as the height? At the moment, we don't expose any function that lets the user manually change the height.

I agree it's not the prettiest solution. I experimented with some different ways and couldn't find something that I was delighted with.

What would be a use case when the user doesn't want to be in control of the scroll? (i.e. scroll within the app instead of around the iframe element).


If I would design the plugin API from scratch, I'd handle this logic in Storyfront's container instead. The plugin would send messages to the Visual Editor whenever the height changed, and Storyfront would determine whether to use that value in the iframe's height. If the plugin is in modal mode, the messages would be ignored.

There may be another possibility; in the ModalChangeMessage, add a new property:

{
  status: boolean // isModalOpen
  fullHeight: boolean
}

If the modal was opened with fullHeight: true, the Visual Editor will ignore any HeightChangeMessage as long as the modal is open, and instead use auto as the height.

eunjae-lee commented 1 year ago

At the moment, we don't expose any function that lets the user manually change the height.

Thanks for the reminder. I completely forgot about this part. Then this approach makes sense to me.

johannes-lindgren commented 1 year ago

Including you here as well, @BibiSebi, for your perspective too šŸ™‚

johannes-lindgren commented 1 year ago

I made an update: instead of letting the field plugin set the height to auto, the field plugin will simply specify to the visual editor that it wants to occupy the full modal window:

This is the new heigthChange message:

{
  status: boolean // isModalOpen
  fullHeight: true
}

The field plugin can keep sending heightChange messages whenever the height changes, but in modal mode, the visual editor will ignore those messages.

Question:

Should we include the fullHeight property in the loaded message instead? This is the message that is initially sent to the visual editor. This might make more sense, as we won't switch between fullHeight=true and fullHeight=false. fullHeight will always be set to true for new field plugins. (https://github.com/storyblok/storyfront/pull/4024#discussion_r1284687011)

eunjae-lee commented 1 year ago

Should we include the fullHeight property in the loaded message instead?

I've left the reply over there šŸ™ŒšŸ¼

eunjae-lee commented 1 year ago

This is the new heigthChange message:

And you meant, the new modalChange message, right?

johannes-lindgren commented 1 year ago

This is the new heigthChange message:

And you meant, the new modalChange message, right?

Oh yes exactly (toggleModal), I highlighted the wrong message