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

Encapsulate codemirror #1907

Merged
merged 13 commits into from Feb 12, 2024
Merged

Encapsulate codemirror #1907

merged 13 commits into from Feb 12, 2024

Conversation

azmy60
Copy link
Contributor

@azmy60 azmy60 commented Jan 18, 2024

fix #1883

Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

This is a great start, but we're still 'leaking' too much of codemirror into the parent components. More of that work needs pushing down into text-editor.

Also, while I know we do this in a couple of places because it is unavoidable, we should not call methods directly on child components.

apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/common/TextEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/tableview/EditorModal.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

This looks really really good. I think this simplifies the codebase a lot.

That said, this PR still does some things that aren't really in the spirit of how Vue expects components to communicate with each other (editorInterface).

If we can remove editorInterface I think we'll be good to merge.

apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Outdated Show resolved Hide resolved
apps/studio/src/components/TabQueryEditor.vue Show resolved Hide resolved
apps/studio/src/lib/editor/languageData.ts Show resolved Hide resolved
@azmy60
Copy link
Contributor Author

azmy60 commented Jan 31, 2024

@rathboma I removed the editorInterface. I'm finally able to update stuff just by props. But some parts really depends on the CodeMirror instance, like removeQuotesFromQuery, autoComplete, etc.. So I put these ones as plugins. Don't know if you'd like it or not, but let me know if it needs some changes.

Copy link
Collaborator

@rathboma rathboma left a comment

Choose a reason for hiding this comment

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

Fantastic work here. This was a LOT of refactoring.

@rathboma
Copy link
Collaborator

rathboma commented Feb 6, 2024

@azmy60 Just need to resolve the merge conflicts, then merge away

@azmy60 azmy60 merged commit e0b7e01 into master Feb 12, 2024
4 checks passed
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.

REFACTOR: Make a 'texteditor' component to encapsulate CodeMirror
2 participants