Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Block comparison: show difference between invalid block conversion options #7995
When you have an invalid block the result of the available actions can be unknown.
This PR adds a 'compare conversion' option that shows the actual HTML difference when 'convert to blocks' is applied.
The difference appears in a modal and the user can select to keep the current contents or apply the conversion, with full knowledge of the result of their action.
The modal adjusts to the height of the block:
Overflowing, if too tall, and scrollable:
And adjusts accordingly on smaller screens (using overflow scroll if too tall):
Note: this is based on the discussion in #7604
How has this been tested?
Types of changes
This adds a new feature.
Noting that the comparison currently uses code from
While Gutenberg does have it's own HTML parsing, and also HTML comparison (via
An alternative is to re-implement
Open to suggestions.
In particular I assumed that we would want to show deleted characters in the right panel, and I've included these in red. We can not show those at all, if it makes more sense.
@johngodley This is impressive work. I think it's super duper solid, and it's VERY much good enough to get into master so we can iterate.
I pushed a small fix so it only scrolls when necessary, and I made the menu option titlecase.
Can you rebase this?
Question: does this use the modal API? Assuming yes as it really seems to be, which is great, just verifying.
changed the title from
WIP - Block comparison: show difference between invalid block conversion options
Block comparison: show difference between invalid block conversion options
Sep 5, 2018
If you mean
Rebased. There's a codecov failure which I don't understand as the unit test coverage has increased.
I've noticed some problems with a few of the more esoteric blocks so I'll look at fixing those.
I made a some small tweaks to the CSS regarding line height. If you had a deleted section wrapping over lines it could overlap:
This has now been fixed.
I noticed two other problems:
Yeah this is a separate issue that would be sweet to fix.
You rule the world.
@johngodley - This is really cool. In fact, I think it's so useful I was wondering if we would want to make it more prominent?
I added some comments, I think the main one that needs to be fixed is the
max-height: 70% issue which is visible to the user.
Will probably also need someone more knowledgeable to comment on the library inclusion. Not sure who would be best.
Thanks for the review @talldan. I do wonder if it might make more sense if the 'convert to blocks' button just defaults to showing the comparison component directly, and we remove the option from the dropdown menu?
The effect of 'convert to blocks' is unknown to the user, and sometimes quite arbitrary and destructive. At the point the user is being presented with this warning I don't think it's excessive to add an additional click in the process. It saves having to undo the action and being annoyed at Gutenberg if the conversion isn't as expected.
@johngodley This is looking good. I've spotted one additional thing here:
I think the only thing we need now is to confirm that the script/dependency inclusion is ok. (edit: worth noting that at least if we use an npm package, the licence is checked by the licence-checker script: #8808).
On the user experience, I feel like it could definitely be iterated on this so that it's part of the workflow when editing a block HTML:
There are other times when the user might end up with invalid html (e.g. using the classic editor), so we might present that slightly differently to what I've proposed above. Would be good to get additional input on this though.