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 text hard wrapping #891

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

JLiu1272
Copy link

@JLiu1272 JLiu1272 commented Mar 28, 2022

Worked on this issue - #521

  • All codemirror-based editors support this behavior. Hence, this feature is supported in /code and /markdown.
  • This feature can be enabled/disabled in settings. If it is enabled, you can set the width at which the text will wrap.

image

Test Cases

  • Check if disabling this feature work as expected. Tested by disabling it in setting, and seeing if /code /markdown actually works without wrapping.
  • Check if enabling this feature work as expected. Tested by disabling it in setting, and seeing if /code /markdown actually works with wrapping.
  • Toggled the width to see if it works as expected. Tested by changing the width in setting, and console logged setting to see if the width was set correctly. Repeated this test for both /code and /markdown

@JLiu1272 JLiu1272 changed the title Attempting to integrate codemirror wrap/hardwrap addon. Add text hard wrapping Apr 4, 2022
// If enable hard wrap max width is enabled, then disabled=false
// otherwise, disabled=true
$input.attr('disabled', !this.checked);
common.setAttribute(['codemirror', hardWrapMaxWidthEnabledKey], this.checked);
Copy link
Author

Choose a reason for hiding this comment

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

Made it so that when this feature is disabled, the input to change the width for wrapping is also disabled, and vice versa.

@JLiu1272 JLiu1272 marked this pull request as ready for review April 4, 2022 01:36
@ansuz
Copy link
Contributor

ansuz commented Apr 11, 2022

Hi,

I finally got around to reviewing and testing this in more depth. I made some changes in the process. I added some comments (via GitHub's UI, not in the code) which explain my reasoning.

This PR does exactly what it's supposed to do when editing alone, however, I found that it introduces some problematic behaviour in collaborative editing sessions. The on('change', ... handler is triggered when a change is made to the editor, but that doesn't distinguish between changes you made and those which were made by remote editors.

The consequence of this is that if one person joins an editing session with hard-wrapping enabled their client will wrap lines according to their preferences any time anybody makes a change, which could be very disruptive.

Another way to approach this is to allow users to set their preferred wrapping width in settings and to apply that to documents that they create, but to provide an additional setting for individual documents that

  1. enables this to be easily enabled/disabled.
  2. ensures that clients agree on the formatting of the shared document

We do this to some degree for the author colors functionality.

@JLiu1272
Copy link
Author

Hi, sorry, got wrapped up with other things. Thanks for the feedback!

@ghost ghost added the Code Related to the Markdown Code app label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Related to the Markdown Code app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants