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(sql-editor-minor-improvements) #3173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alecsci
Copy link
Member

@alecsci alecsci commented Nov 20, 2024

Progresses: 3131

  • disabled spellcheck
  • moved the sql on right top side

- disabled spellcheck
- moved the sql on right top side
@alecsci alecsci changed the title Progresses: 3131 improvements(sql-editor) Nov 20, 2024
@alecsci
Copy link
Member Author

alecsci commented Nov 20, 2024

Hi @FrancescoBorzi, @Helias

For the syntax coloring, I would need some guidance.

I found highlight.js, but not sure if it's integrating correctly with the existing component lifecycle.
I need to register which language is supported, I suppose in the constructor of the sql-component-editor.ts ?

Most of the libs do not provide syntax coloring for text-area, they do it for other html elements.

@alecsci alecsci changed the title improvements(sql-editor) feat(sql-editor-minor-improvements) Nov 20, 2024
@TheSCREWEDSoftware
Copy link
Contributor

@alecsci They are using ngx-highlightjs (which is based on highlightjs) from what i talked to helias a few weeks ago. I haven't had time to try in a basic/blank TS page the textarea synatax colour (which it supports i think reading somewhere in that website or in another post refering to it, if i find i can leave it here)

@alecsci
Copy link
Member Author

alecsci commented Nov 21, 2024

I got it now, they are already using ngx-highlightjs but in other parts of the application.
Thanks @TheSCREWEDSoftware for the hints.

L.E: At this point, I don't know how to properly do this, there are some limitations that must be considered:

  1. can't use a textarea, we need a different html element
  2. two-way binding must be maintained
  3. the ngx is a reactive component, and the code becomes very convoluted
  4. there has to be some programmatic binding can't use anymore the [(ngModel)]

@FrancescoBorzi
Copy link
Collaborator

@alecsci sorry it took a while to review this PR but I'm extremely busy lately. The code looks good. I was wondering if this is already some improvement we could merge or to be considered yet a Work In Progress (I haven't tested it locally, just checked the code).

If you tested this already and it already makes it a bit better, I'd just merge it.

@alecsci
Copy link
Member Author

alecsci commented Dec 17, 2024

Hi @FrancescoBorzi
Yes, I tested it only locally and works, it doesn't completely finish the requirements of the issue, it progresses most of it.

The only missing part to be completely done is the sql code ngx highlighting in the editor menu which I couldn't find yet a solution.

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.

3 participants