-
-
Notifications
You must be signed in to change notification settings - Fork 648
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
improve DiffField.jsx with better support for displaying HTML elements such as images #6309
Conversation
✅ Deploy Preview for plone-components canceled.
|
@dobri1408 having the .snap test files changed means this is a breaking change to the code. I wonder which of your logic makes it so that the content is without the span tag. I would also like to see a before and after of the actual volto stock demo instead of the EEA website diffing in action. |
@ichim-david I made a change by removing the DefaultView for the RenderBlocks (https://github.com/plone/volto/pull/6309/files#diff-1d656037a4db345e1154459194c480148b2b6a85acf74691f76a113f5df89199L81). The reason for this is that the DefaultView also renders elements like the footer, which is not ideal. We want to focus solely on the differences in the blocks. This is why one span tag is removed. Here are some screenshots : |
@dobri1408 it seems to me now that the right side doesn't highlight in green what's new, you have what's removed in the left side but not what it added in the right side. technically speaking the image in the left is also missing from the right so it should also have a red background or border color. |
@ichim-david The coloring is controlled by the CSS style found here:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dobri1408 @ichim-david I tried it out, and it works quite well!! I just tried a couple of things, using teasers etc, but LGTM.
@davisagli @ichim-david if you give your final blessing, we can merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneridagh I am fine with these changes, it's what we had in our EEA projects for quite some time and we haven't received any bug reports as of late.
@dobri1408 I think this should be a .feature instead of bugfix since it provides enhancements to the diff functionality.
The HistoryDiff requires a redesign, as the current layout is broken on complex pages and isn't functioning as expected. My plan is to avoid treating the HTML as a simple string and layering classes on top, which could cause issues. Instead, I'll take a more structured approach by properly parsing the HTML.
Before:
After: