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

CodeMirror Element #2775

Closed
wants to merge 3 commits into from
Closed

CodeMirror Element #2775

wants to merge 3 commits into from

Conversation

frankvp11
Copy link
Contributor

This is the PR that we discussed in #2760. Currently there are a few issues which I list below:

  1. Sometimes it doesn't properly load all the resources, leading to the element not working properly - this can also be seen in the console, as it says CodeMirror is not defined
  2. It currently does not yet support various things like
    -Folding functions
    -Autocomplete
    and various other features that are supported.

Any help in making this work would be much appreciated.
One thing to note is that despite the fact that it logs that you are trying to fold it (when you do ctrl + ',' on the spot) it can't seem to figure out how to collapse it. The way that I am getting inspiration for this is from here https://codemirror.net/5/demo/folding.html. I realize that this is version 5, but I couldn't seem to figure anything else out that was semi-clear.

@falkoschindler
Copy link
Contributor

Thanks, @frankvp11! I just fixed a few minor things, but it already looks quite nice. 🙂

@falkoschindler falkoschindler added the help wanted Extra attention is needed label Mar 29, 2024
@frankvp11
Copy link
Contributor Author

I agree. I have yet to figure out how to get the other features working, as unfortunately there aren't many good examples of how you could integrate them in version 6.
I've also taken the liberty to start working on an Monaco integration aswell. While this one is more limited in terms of future features, it is ultimately easier to implement things.
Some of the differences in my the two elements (Monaco and CodeMirror) is as follows:

  1. CodeMirror has the ability (potentially) to support collaborative editing.
  2. Monaco has minimaps
  3. While possible in CodeMirror, code folding comes out of the box with Monaco despite being difficult to implement (yet to do it) in CodeMirror.
  4. Monaco has find and replace, and bracket/string matching "out of the box"

For the Monaco editor, please let me know if you want that in a seperate PR or if it can go in this PR aswell (as it pertains to code editors).

@falkoschindler
Copy link
Contributor

I think it's better to create a separate PR for Monaco, because it is a separate feature after all and might get merged before or after the CodeMirror element (or not at all). Both elements can get pretty complex, so keeping the PRs reasonably small helps to focus the development and review process.

@chrschorn
Copy link
Contributor

chrschorn commented Apr 14, 2024

I would be interested in helping out here. Do we need/want all features supported by codemirror for a first version of this? I think even without e.g. folding this would be a really helpful component to have in NiceGUI as long as it is reliable (top comment says "Sometimes it doesn't properly load all the resources, leading to the element not working properly").

@rodja
Copy link
Member

rodja commented Apr 15, 2024

I agree with @chrschorn. A minimal working version should be the goal of this PR. We can add code folding, auto-completion etc in later additions.

@frankvp11
Copy link
Contributor Author

I agree also. I think to solve the problem of the modules not loading properly, we should download the files similar to how we did in #2054 and then use the loadResource function. That should get rid of the issues that I was facing earlier where it would take a few refreshes for the issues to go away

@chrschorn
Copy link
Contributor

@frankvp11 I could spend some time looking into this later this week. Let me know if that makes sense or if you're working on this right now.

@frankvp11
Copy link
Contributor Author

That would be great thanks. I've got exams for the next 2 weeks so my development time has gone to zero 😅

@rodja
Copy link
Member

rodja commented Apr 26, 2024

I think we can close this in favour of #2913 which hopefully gets merged for the next release. What do you think @frankvp11?

@frankvp11
Copy link
Contributor Author

I don't think you should have even bothered asking me 😅.
I think it's a great idea- I've been following the development there and it looks amazing.
What do you want to do with #2784, considering their similarity? Perhaps we can just archive it?

@chrschorn
Copy link
Contributor

Thank you for getting this started btw @frankvp11!

@rodja rodja closed this Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants