Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wordpress-playground-block: Require Activation Before Loading Playground Iframe #183

Merged
merged 10 commits into from
Mar 15, 2024

Conversation

brandonpayton
Copy link
Member

What?

This PR updates the wordpress-playground-block to only load Playground after an Activate button has been clicked.

Why?

IFrames can create difficulty for those using Screen Readers. See: #, #

Requiring user action to load Playground will help avoid distracting screen readers with auto-loading iframes. As a bonus, it would also stop the block from automatically consuming network bandwidth to download Playground assets, even if Playground will not be used.

How?

This button must be clicked before the code editor and Playground load, so the iframe no longer loads immediately.

Questions

Would it be better to just gate the iframe with an Activate button? Perhaps that doesn't make sense because of the expected interaction with the code editor, but I think it might look better than a single activation button in the middle of a large block like this one.

@brandonpayton
Copy link
Member Author

This is a work in progress. At least the presentation needs improved, if nothing else.

Here's how it currently looks:
image

@brandonpayton
Copy link
Member Author

A couple of thoughts on this:

Maybe the entire container should be made clickable to activate with a message like "Click to Activate WordPress Playground" or something like that.

Let's add a block setting like "Require Activation" to control this behavior, defaulting to ON.

@brandonpayton
Copy link
Member Author

Maybe the entire container should be made clickable to activate with a message like "Click to Activate WordPress Playground" or something like that.

I am also exploring whether it makes more sense to just require activation of the Playground iframe itself instead of requiring activation for the whole block.

<Button
className="wordpress-playground-activate-button"
onClick={() => setActivated(true)}
>Activate WordPress Playground</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
>Activate WordPress Playground</Button>
>Activate live preview</Button>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @adamziel! That describes the intent much better.

Should "live preview" be capitalized like "Activate Live Preview" or do we typically lowercase subsequent words on buttons?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the capitalization! And let's also add aria attributes to communicate this will create an iframe with a full WordPress site embedded in it, and that it may be a challenge for screen readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 That sounds good.

@brandonpayton
Copy link
Member Author

I am also exploring whether it makes more sense to just require activation of the Playground iframe itself instead of requiring activation for the whole block.

If we do this, then the "Run" button of the code editor doesn't immediately have a loaded iframe to interact with. But we can make the Run button auto-activate the iframe and run when the iframe is loaded.

@brandonpayton brandonpayton force-pushed the add-activation-button branch from c9f62a4 to 472234a Compare March 13, 2024 22:22
@brandonpayton
Copy link
Member Author

brandonpayton commented Mar 13, 2024

Current rendering when activation is limited to iframe area:
image

The code editor seems like it should be taking the full vertical space down to the bar containing the "Run" button. Will look at fixing this in a separate PR.

And the area with the activation button seems too empty and like its are needs to be more visually distinct from the background. Also, when cursor hovers over the Activation button, the button's background changes to the same color as the container background.

@brandonpayton
Copy link
Member Author

Activation button position is currently off in the editor:
image

@brandonpayton
Copy link
Member Author

The off-center button was due to a caching issue. The button's background is transparent in the edtor though, and that needs fixed.

I added a block setting "Require live preview activation" and defaulted it to true.
image

Here is the setting when disabled:
image

The language could be improved. So far, I've found it challenging to keep the text descriptive yet brief.

@brandonpayton
Copy link
Member Author

I added an ARIA description to the Activate button:

This button creates an iframe containing a full WordPress website which may be a challenge for screen readers.

It is untested in an actual Screen Reader, so I made a note to do that.

@brandonpayton
Copy link
Member Author

brandonpayton commented Mar 14, 2024

What appears to be left on this PR:

  • Fix styles
  • Consider improved language for new block setting
  • Test Activation button with screen reader
  • Make code editor Run button trigger activation and run the pending code once the preview is ready

@adamziel
Copy link
Collaborator

adamziel commented Mar 14, 2024

FYI @alexstine. There's still some work left to do here, but I just wanted to share the "activate preview" button for the WordPress Playground block is on its way. Nothing actionable for now :-)

This button must be clicked before the code editor and Playground load.
This will help avoid:
- distracting screen readers with auto-loading iframes
- auto-using network bandwidth to load Playground, even if it is not
  used.
Render the rest of the block as usual.
@brandonpayton brandonpayton force-pushed the add-activation-button branch from fd9dafe to 6a84011 Compare March 14, 2024 20:28
@brandonpayton
Copy link
Member Author

What appears to be left on this PR:

  • Fix styles
  • Consider improved language for new block setting
  • Test Activation button with screen reader
  • Make code editor Run button trigger activation and run the pending code once the preview is ready

These should all be addressed. I believe this PR is ready for review.

@adamziel adamziel added Enhancement New feature or request Playground block labels Mar 15, 2024
@adamziel
Copy link
Collaborator

This is great, thank you so much @brandonpayton! The only note I have is related to the "Require live preview activation" switch in wp-admin. It works, but changing its value doesn't update the live preview in the editor. I thought it was broken for a second:

CleanShot 2024-03-15 at 13 15 35@2x

Other than that, this is solid. I'll go ahead and merge and let's address the preview problem in a follow up PR. Thank you so much @brandonpayton!

@adamziel adamziel merged commit df42e9a into WordPress:trunk Mar 15, 2024
2 checks passed
@brandonpayton brandonpayton deleted the add-activation-button branch March 15, 2024 12:57
@brandonpayton
Copy link
Member Author

Other than that, this is solid. I'll go ahead and merge and let's address the preview problem in a follow up PR.

Thank you, @adamziel!

The only note I have is related to the "Require live preview activation" switch in wp-admin. It works, but changing its value doesn't update the live preview in the editor. I thought it was broken for a second:

I wrestled with what to do here and settled on the current, confusing behavior. 😅 I agree that we should fix it and created an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request Playground block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants