josdejong / svelte-jsoneditor

A web-based tool to view, edit, format, repair, query, transform, and validate JSON
https://jsoneditoronline.org
Other
818 stars 108 forks source link

feat: add indentation on wrapped lines #295

Closed fauzi9331 closed 9 months ago

fauzi9331 commented 11 months ago

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 commented 11 months ago

Thanks @fauzi9331 , that looks promising!

I expect to review your PR after the holiday period.

josdejong commented 10 months ago

@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 commented 10 months ago

@fauzi9331 did you see my comments of last week?

fauzi9331 commented 10 months ago

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 commented 10 months ago

Sounds good 😎

fauzi9331 commented 9 months ago

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

Here's the update:

image

josdejong commented 9 months ago

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 commented 9 months ago

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

fauzi9331 commented 9 months ago

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

josdejong commented 9 months ago

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

fauzi9331 commented 9 months ago

@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 commented 9 months ago

Thanks!

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