Skip to content

Update Visual Editor to support custom preview devices/styles#1

Open
jcheringer wants to merge 4 commits intotrunkfrom
add/custom-preview-devices-support
Open

Update Visual Editor to support custom preview devices/styles#1
jcheringer wants to merge 4 commits intotrunkfrom
add/custom-preview-devices-support

Conversation

@jcheringer
Copy link
Owner

@jcheringer jcheringer commented Sep 9, 2022

Those are the minimal necessary changes I found to allow us to add custom device types.
I think the 'device type' term isn't the best description for this. 'Preview type' seems more descriptive, IMO, but I didn't want to mess with the already used names.
With these changes, we'd be able to do something like this on our side:

	const { setDeviceType, setDeviceStyle } = useDispatch( 'isolated/editor' );

	setDeviceStyle( 'iframe-popup', {
		width: '400px',
		minHeight: '400px',
		background: 'red',
	} );

	setDeviceType( 'iframe-popup' );

Using the term iframe in the device type name would indicate that this preview should render inside an iframe.

Copy link

@ice9js ice9js left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks decent, although I believe the deviceType setting should be independent from deviceStyles.
It feels to me like shouldIframe really is the problem here, that it's hardcoded to deviceType, so maybe that's what we need to change? But I'm also open to ideas.

};
let animatedStyles = desktopCanvasStyles;
if ( resizedCanvasStyles ) {
if ( deviceStyle ) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to overwrite animatedStyles or just augment it?

<MaybeIframe
shouldIframe={ deviceType === 'Tablet' || deviceType === 'Mobile' }
shouldIframe={
deviceType === 'Tablet' || deviceType === 'Mobile' || deviceType.indexOf( 'iframe' ) !== -1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little hacky 🤷 But then we don't want to hardcode things either...
How about if we added a dedicated setting for shouldIframe and deviceType = 'Tablet' | 'Mobile' just overrides that.

We actually want to keep the deviceType's as they are and use the styles in addition to it. As in, ideally, you should still be able to preview a popup on the desktop, tablet or mobile.
Hence we maybe don't want to actually set a deviceType for our purposes.

@jcheringer
Copy link
Owner Author

Thanks for taking a look, @ice9js! 🙇
I changed things a bit based on your comments.
Could you take a look a let me know if it's more aligned with your thoughts?

Copy link

@ice9js ice9js left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments