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

feat: add indentation on wrapped lines #295

Merged
merged 1 commit into from
Sep 13, 2023
Merged

Conversation

fauzi9331
Copy link
Contributor

resolves #228

Hi, in this PR, I've created a custom decoration to add 'padding-left' and 'text-indent' to each line so that the wrapped lines also have indentation.
I used '@replit/codemirror-indentation-markers' as a reference to build this decoration and even copied one of their utility functions, getVisibleLines.
I'm not sure if I put the file in the correct directory, nor am I sure what tests I should do to ensure that I don't break anything. If you can advice, that would be great

image

@josdejong
Copy link
Owner

Thanks @fauzi9331 , that looks promising!

I expect to review your PR after the holiday period.

@josdejong
Copy link
Owner

@fauzi9331 I have tried out your PR and it works like a charm, really nice! It is really much nicer for the eye. I've also tried with large documents and I don't see any performance problems or things that break.

Some thoughts:

  1. I can imagine this is a feature that quite some people would like to have in codemirror. Would it be an idea to publish this as a standalone plugin for CodeMirror, similar to @replit/codemirror-indentation-markers?
  2. Is it possible to let this plugin respect the configured indentation for the editor? It is now hardcoded with an indentation of 2, but if a user configures the editor with 4 spaces instead it would be nice if the wrapped lines adhere to that setting too.
  3. The directory where you've put wrappedLineIndentation.js makes sense to me.
  4. Can you resolve the conflict with the main branch? (it's a simple one)

@josdejong
Copy link
Owner

@fauzi9331 did you see my comments of last week?

@fauzi9331
Copy link
Contributor Author

Hi @josdejong sorry for not replying sooner. Thank you for following up.
I really appreciate the feedback, let me try to publish it as a standalone plugin, this will be my first published package.
I'll also address the other points and update the PR.

@josdejong
Copy link
Owner

Sounds good 😎

@fauzi9331
Copy link
Contributor Author

Hi @josdejong sorry for taking a long time. I've been busy with work and studies lately

Here's the update:

  • Published wrappedLineIndentation.js as a standalone plugin.
  • Ensured the plugin respects the indentation setting.
  • Resolved the conflict

image

@josdejong
Copy link
Owner

Thanks a lot @fauzi9331 , this looks very neat! The configurable indentation indeed works like a charm.

I see there is a linting issue and a failing test due to an outdated snapshot, I'll fix that right away.

@josdejong josdejong merged commit 367accf into josdejong:main Sep 13, 2023
@josdejong
Copy link
Owner

Question: any reason why codemirror-wrapped-line-indent is at version 0.0.2? Is it not version 1.0.0 worthy already?

@fauzi9331
Copy link
Contributor Author

Hi @josdejong no reason for it. I just chose to start from 0 😆

@josdejong
Copy link
Owner

Can you publish it as v1.0.0? That indicates that it is mature and feature complete rather than "your first commit".

@fauzi9331
Copy link
Contributor Author

@josdejong I have bumped the version to 1.0.0. there's no change in the code. do you want me to make new PR to update the version here too?

@josdejong
Copy link
Owner

Thanks!

I'll update all dependencies including codemirror-wrapped-line-indent before the first next release, that will be alright.

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.

long string overflowed the vertical gray line when wrap
2 participants