feat(sandbox,lib,demo): modal in portal #416

johannes-lindgren · 2024-10-03T08:30:43Z

What?

There is an issue that makes various elements on the screen overlap the field plugin modal:

This happens because the field plugin is not using Teleport, but instead uses dynamic styling on its wrapping element to go from position: relative to position: absolute. But the iframe (and its wrapping div element) might very well not have the highest priority in its stacking context. The way this is normally solved in dialogs is to use <Teleport> to mount the element outside the stacking context, (somewhere next to the application root element).

However, this is not possible to implement without some kind of a breaking change: without the Teleport, the iframe element never remounts, which means that the plugin's application state is preserved between. transitions between modal and non-modal modes. But if we conditionally render it inside a <Teleport>, the iframe element is going to remount, which reset the plugin's application state. Some plugins persist their entire app state in the content—but not all, which is why this would be a breaking change.

Rather than making the breaking change in Storyfront, we are introducing an option enablePortalModal in @storyblok/field-plugin which is sent to Storyfront on initialization. This means that the clients will have to manually enable the flag to access this bug ifx. This means that the bug with the overlapping element will persist for all existing field plugins until the authors upgrade and enable the new option.

Changes

The solution is to include a new option enablePortalModal in the field-plugin SDK: when enabled, and when the field plugin loads, it will include a property enablePortalModal: true in the message to initialize. When set to true, the modal will be done with a <Teleport>, otherwise in the old way.

This value is then combined with the isModal state (which can be changed by the field plugin or the editor) into a derived value modalState that is in either of the following states: 'non-modal' | 'modal-with-portal' | 'modal-without-portal'.

The option enablePortalModal is set to true in all templates.

How to test?

#420 makes it more difficult to test. The sandbox preview won't work because it's not a whitelisted host. So you need to serve the sandbox and demo apps in local development, and update packages/demo/src/components/FieldPluginDemo.tsx:

  const { type, data, actions } = useFieldPlugin({
    validateContent,
    // Storyfront in local development mode
    // targetOrigin: 'http://localhost:3300',
    // Sandbox in local development mode:
    targetOrigin: 'http://localhost:7070',
  })

Sandbox

In local development mode, you need change the demo application's targetOrigib:

packages/demo/src/components/FieldPluginDemo.tsx

export const FieldPluginDemo: FunctionComponent = () => {
  const { type, data, actions } = useFieldPlugin({
    validateContent,
    targetOrigin: 'http://localhost:7070',
  })

Then either test with a plugin on the previous versions, or just comment out this line:

packages/field-plugin/src/messaging/pluginMessage/pluginToContainerMessage/PluginLoadedMessage.ts:

The line enablePortalModal: true, in

export const pluginLoadedMessage = (
  options: Pick<PluginLoadedMessage, 'uid' | 'callbackId'>,
): PluginLoadedMessage => ({
  action: 'plugin-changed',
  event: 'loaded',
  fullHeight: true,
  subscribeState: true,
  enablePortalModal: true,
  ...options,
})

vercel · 2024-10-03T08:31:05Z

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	Oct 16, 2024 0:49am

eunjae-lee · 2024-10-04T09:59:06Z

Is it worth making a break change and bump the major version? I wouldn't do that until we have real major improvements. It can give false impression to users.

johannes-lindgren · 2024-10-04T13:48:49Z

Is it worth making a break change and bump the major version? I wouldn't do that until we have real major improvements. It can give false impression to users.

I was contemplating whether we should pass enablePortalModal as an argument to createFieldPlugin. That would benefit all plugins that gets created from this point onward because we could set it as default in the starters. But the drawback with that approach is that we don't force older plugins to deal with the issue. It also required more code, as we'll need to pass the parameters around, even when that parameter should be the default.

johannes-lindgren · 2024-10-08T13:28:07Z

What do you think, @demetriusfeijoo, @Dawntraoz, @BibiSebi, @eunjae-lee? Major or minor version?

demetriusfeijoo · 2024-10-08T14:20:06Z

Hey @johannes-lindgren, sorry for the late reply.

I think we could go as you said, but just release a minor version and update our starters/templates to include enablePortalModal: true by default, so most of the new plugins created based on them are already covered.

Then, In the future, if a new feature introduces an unavoidable breaking change, we can enjoy the opportunity and make the enablePortalModal prop always true by default in the SDK. 🤔

johannes-lindgren · 2024-10-09T09:35:19Z

With a non-breaking change, how do we push users to upgrade? If the release is minor, users are going to increase the version to the latest and call it a day.

BibiSebi · 2024-10-09T09:53:22Z

As it is not a breaking change I would stick with minor. What we can think about is to share this change in our discord. Or even think about opening up discussions (https://github.com/storyblok/field-plugin/discussions/landing) to announce these changes. I would not force them. If they encounter the issue, they will search for answers in our docs/community channels anyway).

johannes-lindgren · 2024-10-09T13:53:44Z

As it is not a breaking change I would stick with minor. What we can think about is to share this change in our discord. Or even think about opening up discussions (https://github.com/storyblok/field-plugin/discussions/landing) to announce these changes. I would not force them. If they encounter the issue, they will search for answers in our docs/community channels anyway).

I think the users are more likely to catch this bug, and they will report it via helpdesk.

johannes-lindgren · 2024-10-09T14:25:19Z

@demetriusfeijoo, @BibiSebi, @Dawntraoz, I added an option to createFieldPlugin.

@BibiSebi, is it okay to update the templates like this in this PR? They are fetched from GitHub right? It it fetched with a tag? I mean, if we upgrade the core library, do we need to release a new version of the CLI as well?

Once this is released, we will need to upgrade all of Storyblok's field plugins that uses modals. I suppose not all of them have been migrated to the new SDK, for example, the table and annotated image plugins?

These are the ones I can think of:

Ecommerce
Bynder
Table (used by customers)
Annotated image (not sure how important)

Did I miss some?

demetriusfeijoo · 2024-10-09T22:22:29Z

Hey @johannes-lindgren 🙌

I started reviewing it but didn't manage to finish it yet.
I'll focus on this and the one on the Storyfront side tomorrow.

These are the ones I can think of:
Ecommerce
Bynder
Table (used by customers)
Annotated image (not sure how important)
Did I miss some?

Oh, I didn't know about Table and Annotated image.
From the top of my mind, I'd just include the Picker Starter on that list.

packages/demo/src/components/FieldPluginDemo.tsx

BibiSebi · 2024-10-10T15:45:04Z

After reviewing the changes on Storyfront, I see that major version indeed might be a better solution here. Let's go with that but let's discuss before hand if we want to somehow add a changelog or a new section to the readme explaining the breaking change?

Also we need to update our docs with the new option.

CC: @johannes-lindgren @demetriusfeijoo @Dawntraoz

johannes-lindgren · 2024-10-11T10:59:56Z

@BibiSebi, @demetriusfeijoo, @Dawntraoz; should I revert my recent changes that introduced the option? Or should I simply merge this as-is, and then follow up with a separate PR that makes this option the default?

johannes-lindgren · 2024-10-11T11:03:57Z

I know that Bynder doesn't have continuous deployment set up, but do the e-commerce plugins have that?

Also, have we upgraded all the old e-commerce plugins yet?

Dawntraoz · 2024-10-11T12:43:49Z

@johannes-lindgren @BibiSebi @demetriusfeijoo After reading the discussions, these are my 2 cents:

Johannes, can you include the option in the README.md or docs so it is already documented? In that case, I would then merge as-is, release a major version for field-plugin, and then the same for field-plugin-cli afterwards, as per the release guidelines: https://github.com/storyblok/field-plugin/blob/main/CONTRIBUTING.md#release.

For the CD on e-commerce plugins, maybe @demetriusfeijoo can answer surely, but as far as I can see in the repo field-plugin e-commerce, all of them have, but they're still some missing in that repo 🤔

demetriusfeijoo

Hey @johannes-lindgren 🙌

@BibiSebi, @demetriusfeijoo, @Dawntraoz; should I revert my recent changes that introduced the option? Or should I simply merge this as-is, and then follow up with a separate PR that makes this option the default?

I think it could be done in a separate PR 👍

I know that Bynder doesn't have continuous deployment set up, but do the e-commerce plugins have that?

All the e-commerce integrations in the main branch have continuous deployment

Also, have we upgraded all the old e-commerce plugins yet?

Not yet 😢

As Alba mentioned there was one TS error related to the new added option:

I have left a comment about the tests which I think may be wrong but overall, the change looks good to me. 👏 👏

demetriusfeijoo · 2024-10-11T14:56:22Z

...ield-plugin/src/messaging/pluginMessage/pluginToContainerMessage/PluginLoadedMessage.test.ts

+    describe('enablePortalModal', () => {
+      it('is optional', () => {
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: undefined,
+          }),
+        ).toEqual(true)
+      })
+      it('is a boolean', () => {
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: true,
+          }),
+        ).toEqual(true)
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: false,
+          }),
+        ).toEqual(true)
+        const { subscribeState: _subscribeState, ...subWithoutSubscribeState } =
+          stub
+        expect(isPluginLoadedMessage(subWithoutSubscribeState)).toEqual(true)
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: 'false',
+          }),
+        ).toEqual(false)
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: 123,
+          }),
+        ).toEqual(false)
+        expect(
+          isPluginLoadedMessage({
+            ...stub,
+            subscribeState: null,
+          }),
+        ).toEqual(false)
+      })
+    })


Hey @johannes-lindgren 👋

Here, I think it should test the enablePortalModal right?

Yes, I don't know what happened here, heh..

johannes-lindgren · 2024-10-15T08:48:09Z

@johannes-lindgren @BibiSebi @demetriusfeijoo After reading the discussions, these are my 2 cents:

Johannes, can you include the option in the README.md so it is already documented? In that case, I would then merge as-is, release a major version for field-plugin, and then the same for field-plugin-cli afterwards, as per the release guidelines: https://github.com/storyblok/field-plugin/blob/main/CONTRIBUTING.md#release.

For the CD on e-commerce plugins, maybe @demetriusfeijoo can answer surely, but as far as I can see in the repo field-plugin e-commerce, all of them have, but they're still some missing in that repo 🤔

@Dawntraoz, should I revert my recent changes? After the initial feedback, I did add it as an option, and set the default to be disabled, so that it wouldn't be a major version.

I moved the documentation to storyblok.com, so there's nothing in the readme file

BibiSebi · 2024-10-16T11:23:49Z

Update: After a conversation with @johannes-lindgren we thoughts to go for minor version update and first see how users react to it and just provide docs. In case we get complaints and support tickets we can always upgrade to major.

CC: @Dawntraoz @demetriusfeijoo

Dawntraoz · 2024-10-16T11:54:11Z

@johannes-lindgren @BibiSebi @demetriusfeijoo After reading the discussions, these are my 2 cents:
Johannes, can you include the option in the README.md so it is already documented? In that case, I would then merge as-is, release a major version for field-plugin, and then the same for field-plugin-cli afterwards, as per the release guidelines: https://github.com/storyblok/field-plugin/blob/main/CONTRIBUTING.md#release.
For the CD on e-commerce plugins, maybe @demetriusfeijoo can answer surely, but as far as I can see in the repo field-plugin e-commerce, all of them have, but they're still some missing in that repo 🤔

@Dawntraoz, should I revert my recent changes? After the initial feedback, I did add it as an option, and set the default to be disabled, so that it wouldn't be a major version.

I moved the documentation to storyblok.com, so there's nothing in the readme file

If you disabled it and it is just the templates, then indeed, I agree, no need for a major version.

I mentioned the README because I saw @BibiSebi mentioned that it would be nice to include it in case of a breaking change, so it is quite clear for anyone looking for answers. But since it will be optional, and then released as a minor version, can you create a ticket to the docs team already to add the option in the main docs: https://www.storyblok.com/docs/plugins/field-plugins/introduction? 🙏 Thanks in advance!

johannes-lindgren · 2024-10-16T13:00:14Z

@johannes-lindgren @BibiSebi @demetriusfeijoo After reading the discussions, these are my 2 cents:
Johannes, can you include the option in the README.md so it is already documented? In that case, I would then merge as-is, release a major version for field-plugin, and then the same for field-plugin-cli afterwards, as per the release guidelines: https://github.com/storyblok/field-plugin/blob/main/CONTRIBUTING.md#release.
For the CD on e-commerce plugins, maybe @demetriusfeijoo can answer surely, but as far as I can see in the repo field-plugin e-commerce, all of them have, but they're still some missing in that repo 🤔

@Dawntraoz, should I revert my recent changes? After the initial feedback, I did add it as an option, and set the default to be disabled, so that it wouldn't be a major version.
I moved the documentation to storyblok.com, so there's nothing in the readme file

If you disabled it and it is just the templates, then indeed, I agree, no need for a major version.

I mentioned the README because I saw @BibiSebi mentioned that it would be nice to include it in case of a breaking change, so it is quite clear for anyone looking for answers. But since it will be optional, and then released as a minor version, can you create a ticket to the docs team already to add the option in the main docs: https://www.storyblok.com/docs/plugins/field-plugins/introduction? 🙏 Thanks in advance!

Thanks for the clarification. I agree the docs are essential. I will personally update the docs 🙂 (I wrote them: https://www.storyblok.com/docs/plugins/field-plugins/storyblok-field-plugin)

johannes-lindgren added 2 commits October 2, 2024 17:03

feat: introduce usePortalModal to PluginLoadedMessage

f90f984

feat: modal in portal

7ede7bf

johannes-lindgren self-assigned this Oct 3, 2024

vercel bot deployed to Preview October 3, 2024 08:31 View deployment

fix(sandbox): implementation for sandbox

c603fcb

johannes-lindgren changed the title ~~fix(sandbox,lib,demo): modal in portal~~ fix(sandbox,lib,demo)!: modal in portal Oct 3, 2024

vercel bot deployed to Preview October 3, 2024 10:16 View deployment

fix(sandbox): close modal from the parent app

c4f98a6

vercel bot deployed to Preview October 3, 2024 13:08 View deployment

refactor(sandbox): better state management of modal

43b4c30

vercel bot deployed to Preview October 4, 2024 09:20 View deployment

feat: enablePortalModal option

6f591ea

johannes-lindgren changed the title ~~fix(sandbox,lib,demo)!: modal in portal~~ fix(sandbox,lib,demo): modal in portal Oct 9, 2024

vercel bot deployed to Preview October 9, 2024 14:04 View deployment

fix: ts errors

381fd16

johannes-lindgren requested review from demetriusfeijoo and Dawntraoz and removed request for demetriusfeijoo October 9, 2024 14:08

vercel bot deployed to Preview October 9, 2024 14:09 View deployment

johannes-lindgren requested a review from BibiSebi October 9, 2024 14:10

feat: enable by default in templates

5bf71a7

Merge branch 'main' into SHAPE-7187-modal-with-portal-in-field-plugin

768a553

johannes-lindgren marked this pull request as ready for review October 9, 2024 14:20

johannes-lindgren changed the title ~~fix(sandbox,lib,demo): modal in portal~~ feat(sandbox,lib,demo): modal in portal Oct 9, 2024

vercel bot deployed to Preview October 9, 2024 14:26 View deployment

johannes-lindgren commented Oct 10, 2024

View reviewed changes

packages/demo/src/components/FieldPluginDemo.tsx Outdated Show resolved Hide resolved

Update packages/demo/src/components/FieldPluginDemo.tsx

868873d

vercel bot deployed to Preview October 10, 2024 08:58 View deployment

fix: templates

5be7b06

vercel bot deployed to Preview October 10, 2024 09:31 View deployment

demetriusfeijoo reviewed Oct 11, 2024

View reviewed changes

fix: tests

b0d2b9a

johannes-lindgren requested a review from demetriusfeijoo October 16, 2024 08:23

vercel bot deployed to Preview October 16, 2024 08:24 View deployment

BibiSebi approved these changes Oct 16, 2024

View reviewed changes

Dawntraoz approved these changes Oct 16, 2024

View reviewed changes

gustavomelki approved these changes Oct 16, 2024

View reviewed changes

Merge branch 'main' into SHAPE-7187-modal-with-portal-in-field-plugin

42c2765

vercel bot deployed to Preview October 16, 2024 12:49 View deployment

gustavomelki merged commit 1ceeda4 into main Oct 16, 2024
4 checks passed

gustavomelki deleted the SHAPE-7187-modal-with-portal-in-field-plugin branch October 16, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sandbox,lib,demo): modal in portal #416

feat(sandbox,lib,demo): modal in portal #416

johannes-lindgren commented Oct 3, 2024 •

edited

Loading

vercel bot commented Oct 3, 2024 •

edited

Loading

eunjae-lee commented Oct 4, 2024

johannes-lindgren commented Oct 4, 2024 •

edited

Loading

johannes-lindgren commented Oct 8, 2024 •

edited

Loading

demetriusfeijoo commented Oct 8, 2024

johannes-lindgren commented Oct 9, 2024

BibiSebi commented Oct 9, 2024

johannes-lindgren commented Oct 9, 2024

johannes-lindgren commented Oct 9, 2024 •

edited

Loading

demetriusfeijoo commented Oct 9, 2024

BibiSebi commented Oct 10, 2024

johannes-lindgren commented Oct 11, 2024

johannes-lindgren commented Oct 11, 2024

Dawntraoz commented Oct 11, 2024 •

edited

Loading

demetriusfeijoo left a comment

demetriusfeijoo Oct 11, 2024

johannes-lindgren Oct 16, 2024 •

edited

Loading

johannes-lindgren Oct 16, 2024

johannes-lindgren commented Oct 15, 2024 •

edited

Loading

BibiSebi commented Oct 16, 2024 •

edited

Loading

Dawntraoz commented Oct 16, 2024

johannes-lindgren commented Oct 16, 2024

feat(sandbox,lib,demo): modal in portal #416

feat(sandbox,lib,demo): modal in portal #416

Conversation

johannes-lindgren commented Oct 3, 2024 • edited Loading

What?

Changes

How to test?

Sandbox

vercel bot commented Oct 3, 2024 • edited Loading

eunjae-lee commented Oct 4, 2024

johannes-lindgren commented Oct 4, 2024 • edited Loading

johannes-lindgren commented Oct 8, 2024 • edited Loading

demetriusfeijoo commented Oct 8, 2024

johannes-lindgren commented Oct 9, 2024

BibiSebi commented Oct 9, 2024

johannes-lindgren commented Oct 9, 2024

johannes-lindgren commented Oct 9, 2024 • edited Loading

demetriusfeijoo commented Oct 9, 2024

BibiSebi commented Oct 10, 2024

johannes-lindgren commented Oct 11, 2024

johannes-lindgren commented Oct 11, 2024

Dawntraoz commented Oct 11, 2024 • edited Loading

demetriusfeijoo left a comment

Choose a reason for hiding this comment

demetriusfeijoo Oct 11, 2024

Choose a reason for hiding this comment

johannes-lindgren Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

johannes-lindgren Oct 16, 2024

Choose a reason for hiding this comment

johannes-lindgren commented Oct 15, 2024 • edited Loading

BibiSebi commented Oct 16, 2024 • edited Loading

Dawntraoz commented Oct 16, 2024

johannes-lindgren commented Oct 16, 2024

johannes-lindgren commented Oct 3, 2024 •

edited

Loading

vercel bot commented Oct 3, 2024 •

edited

Loading

johannes-lindgren commented Oct 4, 2024 •

edited

Loading

johannes-lindgren commented Oct 8, 2024 •

edited

Loading

johannes-lindgren commented Oct 9, 2024 •

edited

Loading

Dawntraoz commented Oct 11, 2024 •

edited

Loading

johannes-lindgren Oct 16, 2024 •

edited

Loading

johannes-lindgren commented Oct 15, 2024 •

edited

Loading

BibiSebi commented Oct 16, 2024 •

edited

Loading