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

Add diff-view for source translation in sbs-editor #1621

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

david-venhoff
Copy link
Member

@david-venhoff david-venhoff commented Aug 3, 2022

Short description

This pr adds functionality to render the diff of the source translation since the last update of the target translation in the side-by-side page editor.

Proposed changes

  • Render the diff of the source translation in sbs-view
  • Add a button to toggle between the normal view and the diff view
  • Remove disabled tinymce editor instance in source translation for better consistency

Resolved issues

Fixes: #1449, Fixes: #1573

@david-venhoff david-venhoff requested a review from a team as a code owner August 3, 2022 15:49
@codeclimate
Copy link

codeclimate bot commented Aug 3, 2022

Code Climate has analyzed commit d085d41 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 77.7% (50% is the threshold).

This pull request will bring the total coverage in the repository to 74.1% (0.0% change).

View more on Code Climate.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍
This nearly works as expected, I would just adapt the look of the source translation to the target translation to make it easier to compare both sides (so a fixed size even for very short or very long pages would be good and add a bit of margin so they are horizontally aligned). If possible, the disabled tinymce area would solve this because it already looks like the right side without any adaptions, but if this doesn't work well with the html diff, we could try to at least make it somehow similar.

integreat_cms/cms/views/pages/page_sbs_view.py Outdated Show resolved Hide resolved
integreat_cms/cms/views/pages/page_sbs_view.py Outdated Show resolved Hide resolved
integreat_cms/cms/templates/pages/page_sbs.html Outdated Show resolved Hide resolved
@david-venhoff david-venhoff force-pushed the feature/show_sbs_source_diff branch 4 times, most recently from d86ea61 to 1a4901c Compare August 11, 2022 14:54
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Amazing, really cool that the styling works in the tinymce area! 👍
Now I just have a small idea for the button's appearance.

integreat_cms/cms/templates/pages/page_sbs.html Outdated Show resolved Hide resolved
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot for your changes! 👍

Almost looks perfect, but sorry, another thought just crossed my mind: When the diff is viewed in the source editor, and then the source content is copied into the target editor, the complete diff including the red/green formatting is copied, which is probably not what was intended. Do you think we could always copy the new source content, no matter whether the diff is currently shown or not?

@david-venhoff david-venhoff force-pushed the feature/show_sbs_source_diff branch 4 times, most recently from 3043e48 to f49cbec Compare August 14, 2022 09:24
@david-venhoff
Copy link
Member Author

Oh, somehow missed that...
Should be fixed now!

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🙏

I'm sorry about my iterative review loop, but I have just found another problem 😿

Could you inject the old_translation_content template context in both the GET and POST view? At the moment, the diff only works correctly when requesting the form with GET, but after e.g. a submission without changes, the diff does not work correctly...

integreat_cms/cms/views/pages/page_sbs_view.py Outdated Show resolved Hide resolved
integreat_cms/cms/views/pages/page_sbs_view.py Outdated Show resolved Hide resolved
integreat_cms/cms/views/pages/page_sbs_view.py Outdated Show resolved Hide resolved
@david-venhoff david-venhoff force-pushed the feature/show_sbs_source_diff branch 2 times, most recently from 6c2510f to 7327fc3 Compare August 30, 2022 10:52
@david-venhoff
Copy link
Member Author

I have update the pr and the issues with minor edits should be resolved now.

Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Thank you! 👍 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show diff to last version in side-by-side view Show diff to last source version in side-by-side view
2 participants