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

Update invalid block copy #9667

Merged
merged 2 commits into from
Oct 4, 2018
Merged

Update invalid block copy #9667

merged 2 commits into from
Oct 4, 2018

Conversation

johngodley
Copy link
Contributor

Based on feedback in #9473 this tries to improve the text for the content and buttons of an invalid block from:

edit_post_ wordpress_latest _wordpress

To:

edit_post_ wordpress_latest _wordpress

It also changes the text for a single-use block to make it more active. From:

edit_post_ wordpress_latest _wordpress

To:

edit_post_ wordpress_latest _wordpress

Finally it removes the duplicate invalid block button from:

overview_of_block_validation_experience_ issue__7604 _wordpress_gutenberg

To:

edit_post_ wordpress_latest _wordpress

How has this been tested?

This is a text only change.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@johngodley johngodley added [Feature] Blocks Overall functionality of blocks [Type] Copy Issues or PRs that need copy editing assistance Needs Copy Review Needs review of user-facing copy (language, phrasing) labels Sep 6, 2018
@johngodley
Copy link
Contributor Author

The ‘keep’ button still seems a little misleading because ‘keep’ indicates that it remains the same while it actually converts to a HTML block. Maybe ‘Convert to HTML’ would be better?

Suggestions welcome!

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Sep 6, 2018

I agree with @johngodley that Convert to HTML would be better.

Also, I think an Edit as HTML option (which does the same thing as the one in the ellipsis menu) should be added to the ellipsis menu and the ellipsis menu in the toolbar should be removed. It is kind of weird having 2 ellipsis menus shown, and you don't really want to expose other options like duplicating an invalid block.

@0aveRyan
Copy link
Contributor

0aveRyan commented Sep 6, 2018

@johngodley I really like the updated text This block contains unexpected content. -- much friendlier, clearer and less-scary. While I appreciate trying to distill those buttons, I think the original button text are still clearer to users -- perhaps we could wordsmith those some more?

I think Convert to Blocks is pretty solid, but agree there's some confusion with Classic/HTML conversion. Part of me would love Keep as code to get an acronym out, but alas, the Code Block.

Perhaps there's a happy medium that could be achieved by adding descriptive tooltips to your shortened button text?

@ZebulanStanphill
Copy link
Member

I think Convert to HTML Block would probably be the best option for converting to a Custom HTML block; it is self-explanatory and consistent with the Convert to Classic Block option.

@johngodley
Copy link
Contributor Author

Some good suggestions, thanks!

I agree that 'Convert to HTML block' is appropriate and matches 'Convert to Classic Block'. The attempts at brevity are to try and keep everything on one line on desktop (for English and other languages of a similar text length). It's worth trying though.

I think Convert to Blocks is pretty solid

I'm kind of in two minds about it and I'm not sure if my knowledge of the underlying code is influencing this! @talldan has a good point here:

And 'Convert to Block' doesn't always make sense since it's already a block, just one with an warning state.

I went with 'auto-convert' instead (alternative could be 'auto-fix') as it indicates that a conversion is going to happen, and Gutenberg is going to do whatever it can.

Perhaps there's a happy medium that could be achieved by adding descriptive tooltips to your shortened button text?

I think this is a great idea - a tooltip with a longer description could really help explain the finer details.

Also adding that #7995 will add a comparison diff for 'Convert to blocks' so you'll be able to see exactly what happens - this may help.

@0aveRyan
Copy link
Contributor

0aveRyan commented Sep 6, 2018

@johngodley (+ @talldan) that's an excellent point about already being a block -- that is confusing. Perhaps Extract Single Blocks or Parse to Blocks? These retain the suggestion it is a Block, but perhaps don't sound like they're being morphed from some non-Block unknown.

I really like your proposal in #7995 -- giving a diff definitely helps increase confidence in the process.

@talldan
Copy link
Contributor

talldan commented Sep 6, 2018

Something like "Recover Block" might work as well.

edit: Or even just "Recover" / "Convert to HTML Block" as the options.

@michelleweber
Copy link

Both the "modified externally" and "contains unexpected content" messages seems like that have insufficient information for the user. The block has been modified externally: so what? There's unexpected content: what does that mean? It's not unexpected to me; I put it there. Since there's not a lot of information, it's hard for the user to know what they're supposed to do -- we don't give them any guidance on which choice is the right/best choice. And if I'm seeing this message for the first time, will I know what auto-convert/keep mean?

Can we add more context to these messages? Especially since blocks are so new for everyone; that gives us even more reason to provide useful, actionable information in. It would be great to see a real explanation of what is happening and what the choices actually mean, and we can then edit down to a reasonable length.

@0aveRyan
Copy link
Contributor

0aveRyan commented Sep 6, 2018

Good points @michelleweber, here's some ideas trying to include that feedback:

This block couldn't be identified or is broken.
Try Recovery + Keep As HTML

This block couldn't be identified and appears to be HTML code.
Try Block Recovery + Keep As-Is

@johngodley
Copy link
Contributor Author

@michelleweber to answer your question, the message appears when the current content of the block doesn't match what Gutenberg expects it to be.

Usually a block is visually edited, and the HTML output is known - Gutenberg generates the HTML based on the visually edited data.

However, we also allow people to directly edit the HTML (the 'edit as HTML' option). Gutenberg will compare what it thinks the HTML should be against the user changes, and flags an error if there is a difference.

Typically this difference is because the user introduced custom HTML into the block, and so we offer a 'Keep as HTML' option. This converts the block into a HTML block, allowing the user to carry on with their custom content. This is the default button as it results in no data loss. The main difference is they can no longer visually edit the block and must continue to use HTML.

The 'Convert to blocks' or 'Auto-convert' runs the block through Gutenberg's HTML parser in an attempt to determine what block(s) it best matches, and then converts it into valid block(s). This can cause a loss of information as Gutenberg will throw away data it doesn't understand, and it may convert to another kind of block, or even several blocks. An incoming feature in #7995 will visually show this conversion (although it is hidden behind a dropdown menu).

Another reason why the message is triggered is because the user could break the HTML in some way. For example, by removing an HTML tag.

There may be other ways in which the block's HTML is modified outside of the user directly changing it, which I think is the origin of the 'externally modified' text, but I can't think of a specific instance right now.

Hopefully that explains what the message is about. Essentially the user changes a block in such a way that they need to decide whether to move it out of the visual Gutenberg world into HTML, or whether to push it back inside - Gutenberg is very particular in this respect.

@michelleweber
Copy link

All super helpful, thanks y'all.

I like the direction of @0aveRyan 's last suggestion. I might tweak a little to try and go a little friendlier and add a smidge more info.

WordPress can't match this with any block type, or it might be broken. What do you want to do with it?
Try to convert to a block + Keep as HTML

WordPress can't match this with any block type -- it looks like you've entered some custom HTML code. What do you want to do with it?
Try to convert again + Keep as-is

Question: in "it might be broken," what is "it"? Is this saying "it might be a broken block"?

@johngodley
Copy link
Contributor Author

"it might be broken," what is "it"

It's referring to broken/invalid HTML. There's a separate warning, without buttons, that appears if the block's code breaks.

Here's some screenshots of the suggestions to see how they look:

edit_post_ wordpress_latest _wordpress

edit_post_ wordpress_latest _wordpress

edit_post_ wordpress_latest _wordpress

edit_post_ wordpress_latest _wordpress

I'll include @jasmussen in for thoughts about the effect on the design as I think it was originally designed for a single line (for English, at least)

@jasmussen
Copy link
Contributor

jasmussen commented Sep 10, 2018

I know Matías has a lot of thoughts on this.

There are a lot of things to balance — we hope for the user to not see this warning very often. And we also want for the warning to not be too warningy, because something bad has not necessarily happened, honestly.

Finally, because it already looks slightly jarring, I personally feel like the less text, the better. In that vein, I like "Keep as-is" as the 2nd option.

Perhaps we could keep the text ultra small and sort of friendly, like:

"This block wasn't recognized, or might be damaged."

If only there was a softer word for "damaged". Maybe "modified"?

Finally I thought it was an interesting suggestion, I think by @jorgefilipecosta on a different thread, to involve the new diffing tool in this.


So, summary, perhaps the warning could look like this:

This block wasn't recognized, or might be modified. [Compare] [Keep as-is]

Clicking [Compare] opens up the diffing modal which shows the most recent change that happened to the block.

This modal, in other words:

Compare Block Conversion

[Current]                       |           [After Conversion]
                                |
[Keep as HTML]                  |           [Convert to blocks]

How about that?

Then we keep it to two buttons on the warning, we keep the warning simple, the "Compare" warning isn't very scary and the actual diff helps the confidence. And we save an ellipsis menu option!

@talldan
Copy link
Contributor

talldan commented Sep 10, 2018

We're also discussing that on the PR for the diffing tool @jasmussen:
#7995

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Sep 10, 2018

@jasmussen

If only there was a softer word for "damaged". Maybe "modified"?

The word "modified" sounds confusing to me, because aren't you supposed to be able to modify blocks? I think "damaged" is a better choice, though perhaps there is a word better than both.

This block wasn't recognized, or might be modified. Compare Keep as-is

I like this, except that I am not sure that "Keep as-is" is the right wording here. It implies that the block will be left unchanged, which is not entirely true. It does not inform the user that it will be converted into a different block entirely. I think "Keep as HTML" is better.

@talldan
Copy link
Contributor

talldan commented Sep 10, 2018

I still think it all still sounds a bit 'tech-speak', and that a user new to the concept of blocks and Gutenberg probably wouldn't have a clue what's happening when they see this. Especially if the situation is that another user has changed something in the post using the classic editor.

I think the diff screen is fantastic if the user has edited the HTML themselves, but maybe not so much if the error has come from elsewhere (although the visual diff is nice). The user might not be tech-savvy enough to understand the HTML, especially given one of the philosophies of Gutenberg is to keep them away from it .

For that reason, I do quite like a longer sentence, but understand that it looks wordy. I wonder if there's a way we can have a short description and a longer explanation that's initially hidden.

Or a link to an FAQ (e.g. 'Find out more about this warning').

@johngodley
Copy link
Contributor Author

the error has come from elsewhere

I'd actually be interested to know more about this. Other than somebody else editing (and breaking) the post, are there other sources for this message?

It's worth remembering that this error message is probably the result of the user editing HTML. While they may not be very familiar with Gutenberg, we can maybe assume they are confident enough to pick 'edit as HTML' or 'code editor', and then confident enough to mess around with the HTML. As Joen describes, this message is hopefully never shown, but if it is it's likely to be shown to a more advanced user (or at the least, one who knowingly edited and could expect a warning).

There is other work to make this message less trigger-happy, so ideally it will only be seen by someone who can react confidently.

I think we agree the current text is confusing. In an effort to reduce the scope of this PR these ideas seem worth doing elsewhere:

  1. Link to a descriptive FAQ/help page or show button tooltips
  2. Change the Convert to blocks button to Compare, and have it directly show the diff screen

With this in mind, the question remains about improving the current text. This may not be the final text, and we can iterate as other changes improve the experience. Note this is just my attempt at merging everyone's suggestion while trying to stay reasonably compact!

Keep as HTML => Keep as HTML or Convert to HTML

My thought here is that 'convert to HTML' more accurately describes the action, while 'keep' implies it stays the same.

Convert to Blocks => Attempt recovery or Auto convert

I think this needs to indicate that Gutenberg is going to make a best attempt at 'fixing' it. The fact that it may change it into another block, or several blocks, is something that will be covered by (2).

This block has been modified externally => This block is unrecognized or contains modified HTML

I think this probably represents the cause of the warning, and mentioning HTML should remind people that's it's because they edited directly. We can add more descriptive text via (1)

Still very open to changes, and I'll update the code when it settles.

@ZebulanStanphill
Copy link
Member

@johngodley
I think I would prefer "Convert to HTML" since it is more accurate, though "Convert to HTML block" or even "Convert to Custom HTML block" would be the most accurate of all, with the downsides of being longer, of course.

I am fine with either "Attempt recovery" or "Auto convert", but I think I would lean towards the former.

"This block is unrecognized or contains modified HTML"

This implies that modifying HTML is not allowed at all for blocks, so I would change that to say "This block is unrecognized or contains invalid modified HTML" or something like that.

@johngodley
Copy link
Contributor Author

Ok, updated the PR with the latest, based on the assumption that the user has just edited their HTML and has a little context and knowledge:

edit_post_ wordpress_latest _wordpress

It just fits in one line, in English, and is hopefully a little clearer than before. I suspect the text will continue to evolve and adapt to other changes we make.

I've created two related issues, discussed above, which should help further:

Copy link
Contributor

@talldan talldan 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 good to me, great work on all your recent PRs @johngodley! 🎉

@mtias are you also happy with the changes?

@johngodley johngodley added this to the 3.9 milestone Sep 14, 2018
@aduth
Copy link
Member

aduth commented Sep 14, 2018

Minor consistency note:

In the top-level error boundary, we use the string "Attempt Recovery":

AFAICT, "Try" and "Attempt" would be considered synonyms. Any reason they shouldn't be the same, even if that means changing the error boundary to "Try" (which I have no qualms against).

Also a consideration for localization effort in maintaining two separate overlapping strings.

@mtias
Copy link
Member

mtias commented Sep 14, 2018

Nice conversation and thoughts. I have a few points to add:

  • None of the options make sense if I cannot see what they'll produce, so I think adding the "Compare" is crucial.
  • Maybe it can even be renamed to "Resolve", which opens the visual diff and let's you make a more confident decision.
  • I liked "Keep as is" mostly for the flow where you edited the HTML and switched back and it threw an error. It communicates that nothing is inherently bad, just that you might be losing the ability to access certain block controls.
  • For this context "Convert to HTML" seems fine.
  • In the future we might be doing some of these automatically:
    • Detect if there are visual diff changes, and if there aren't, attempt a recovery automatically.

I think this is a good iteration and I'm looking forward to:

#9862 - Invalid block 'try recovery' should show block comparison modal

However, I don't fully like the last text "This block is unrecognized or contains invalid HTML" because it's not true — the block is recognized, which is why we can show the message in the first place.

Something like "There appears to be an issue with the content of this block which we need you to make a call on" is what we are effectively telling the user.

I'd actually be interested to know more about this. Other than somebody else editing (and breaking) the post, are there other sources for this message?

It's common for other editors to modify the HTML without the user knowing. That is right now the source of most of these messages. I actually think the user modifying the source is the least common.

The case where the user switches to "Edit as HTML" and breaks the block is already handled separately as an exception, so that covers most direct edits.

@tofumatt tofumatt modified the milestones: 3.9, 4.0 Sep 14, 2018
Try and make it clearer while not wrapping any buttons in invalid messages
The ‘resolve’ button now directly invokes the diff comparison modal
@johngodley
Copy link
Contributor Author

Tweaked again:

edit_post_ wordpress_latest _wordpress

The new 'Resolve' button now directly opens the comparison modal, as per #9862. The main text has been changed to This block contains unexpected or invalid content., which I think indicates that there's stuff in the block we don't know how to handle.

It's common for other editors to modify the HTML without the user knowing

Wouldn't this trigger the invalid message for the editor?

@tofumatt tofumatt removed the Needs Copy Review Needs review of user-facing copy (language, phrasing) label Sep 29, 2018
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

Reading through this issue the new copy seems in line with what @michelleweber and @mtias suggested, and it makes sense to me.

I don't think anything else needs to be done here; I've removed the "needs copy editing" label because it's been through a few rounds and this seems in line with what was suggested.

I say 🚢

@gziolo
Copy link
Member

gziolo commented Oct 4, 2018

Given that there were no further comments, I'm going to merge this one.

@gziolo gziolo merged commit 557dccc into master Oct 4, 2018
@gziolo gziolo deleted the update/block-warning-text branch October 4, 2018 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Copy Issues or PRs that need copy editing assistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants