Html block: Add option to allow preview by default

[mauriac]

A new option has been added to HTML block to allow 'preview' mode by default

What?

fixes: #40913

Why?

See the issue.

How?

In this PR, a new attribute showPreview has been added to HTMLEdit component, and then when the state of this attribute changes the html view mode changes accordingly.

Testing Instructions

Screenshots or screencast

[amustaque97]

Thank you @mauriac for working on this. I'm thinking do we really need if-else condition? While doing review I observed both the variables isPreview and showPreview are in sync if we modify current statement something like this:

-const [ isPreview, setIsPreview ] = useState();
+const [ isPreview, setIsPreview ] = useState(false);

PS: Current default value of isPreview variable is undefined.

Looking forward to hearing from you

[mauriac]

Hello @amustaque97 thank you for your review, but I am a bit lost;

I do not get the suggestion. Please do you mind enlighten me?

The sync between isPreview and showPreview is just a plus, but on second thought let's remove it.

Thank you.

[amustaque97]

The sync between isPreview and showPreview is just a plus, but on second thought let's remove it.

What I mean to say is that we can remove if-else block and can write something like this

useEffect(() => {
    setIsPreview( showPreview );
}, [showPreview])

Also, to change line here

-const [ isPreview, setIsPreview ] = useState();
+const [ isPreview, setIsPreview ] = useState(false);

I hope I'm able to explain request changes.

Thank you.

[amustaque97]

@mauriac , I saw your changes. I will take a proper look at it in sometime and update my comments here.

[mauriac]

The sync between isPreview and showPreview is just a plus, but on second thought let's remove it.

What I mean to say is that we can remove if-else block and can write something like this

useEffect(() => {
    setIsPreview( showPreview );
}, [showPreview])

Also, to change line here

-const [ isPreview, setIsPreview ] = useState();
+const [ isPreview, setIsPreview ] = useState(false);

I hope I'm able to explain request changes.

Thank you.

Oh yes you are totally right. I got it now, I make some changes, give me your back when you have sometime. thank you.

[amustaque97]

Your changes makes more sense to load preview setting once. Also, can we change useEffect block to look like this:

useEffect( () => {
    if ( showPreview ) {
        switchToPreview();
    }
}, [] );

I think PR is good to approve after the above final changes. Let me know what you think?

[mauriac]

Your changes makes more sense to load preview setting once. Also, can we change useEffect block to look like this:

useEffect( () => {
    if ( showPreview ) {
        switchToPreview();
    }
}, [] );

I think PR is good to approve after the above final changes. Let me know what you think?

I agree, I applied changes, so what about this change
+const [ isPreview, setIsPreview ] = useState(false); should we still apply it?

[amustaque97]

This PR has frontend change I would like to involve @jasmussen and other bunch of folks for design/UX review 🙂

[Mamaduka]

Suggested change

	const [ isPreview, setIsPreview ] = useState();
	const [ isPreview, setIsPreview ] = useState( showPreview );

Suggestion: We can use attribute value as the initial state. It would be similar to the current effect that only runs on the mount.

[mauriac]

Hi @Mamaduka thanks for your review, that works perfectly, but we have a new suggestion in the comments below (here) that blocks me to make new commit now.

[Mamaduka]

Thanks for working on this, @mauriac.

I left one suggestion, but the otherwise feature works as expected.

[mtias]

This shouldn't be an attribute of the block, we should probably look into making preview the default view when the block is unselected.

[mauriac]

This shouldn't be an attribute of the block, we should probably look into making preview the default view when the block is unselected.

Hello @mtias thanks for your comment. I think it makes sense, and I should agree with, but the advantage of the new option here is to allow users to choose what will be good for them, so if we remove the attribute, what happen for those who will not want to get the preview mode by default?

Looking forward to hear you. Thanks

[kohheepeace]

+1 for this feature. And I don't think InspectorControls is necessary.

Also, Instead of using local state like below

const [ isPreview, setIsPreview ] = useState( showPreview );

Call setAttributes directly like below isn't better?

In edit.js

...

export default function HTMLEdit({ attributes, setAttributes, isSelected }) {
  const { isPreview } = attributes;
  ...

  function switchToPreview() {
    setAttributes({ isPreview: true });
  }

  function switchToHTML() {
    setAttributes({ isPreview: false });
  }

  return (
 ...
  );
}

*NOTE: Of course define isPreview attribute in block.json

...
  "attributes": {
   ...
    "isPreview": {
      "type": "boolean",
      "default": false
    }
  },
...

The user's toggled state is saved and remains even if the browser is reloaded.

[kohheepeace]

@mtias

making preview the default view when the block is unselected.

=> I think this will make editor UX bad (crackling? popping? <- sorry I forget English words to describe this phenomenon 😢 )
Because of code height and preview HTML height will maybe different.

[mauteri]

This shouldn't be an attribute of the block, we should probably look into making preview the default view when the block is unselected.

PR for on select approach: #46836