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.

Sign up for GitHub

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

Jump to bottom

[BUG] fix (tab) indentation within CodeMirror #4832

Merged
merged 7 commits into from
Dec 13, 2023
Merged

[BUG] fix (tab) indentation within CodeMirror #4832

Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4cc809b
- tab indent by 4
hasan-sh Nov 30, 2023
2dbe626
unused variables
hasan-sh Nov 30, 2023
1ca714e
code enhancement
hasan-sh Dec 4, 2023
f6715d4
Merge branch 'main' into fix-indentation
mergify[bot] Dec 12, 2023
dbc128c
Merge branch 'main' into fix-indentation
Felienne Dec 13, 2023
ef8a872
Merge branch 'main' into fix-indentation
hasan-sh Dec 13, 2023
5a71a89
Fix tests
hasan-sh Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions static/js/cm-editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { EditorView, ViewUpdate, drawSelection, dropCursor, highlightActiveLine,
highlightActiveLineGutter, highlightSpecialChars, keymap, lineNumbers } from '@codemirror/view'
import { EditorState, Compartment, StateEffect, Prec } from '@codemirror/state'
import { EventEmitter } from "./event-emitter";
import { deleteTrailingWhitespace, defaultKeymap, historyKeymap } from '@codemirror/commands'
import { deleteTrailingWhitespace, defaultKeymap, historyKeymap, indentWithTab } from '@codemirror/commands'
import { history } from "@codemirror/commands"
import { indentOnInput, defaultHighlightStyle, syntaxHighlighting ,LanguageSupport } from "@codemirror/language"
import { indentOnInput, defaultHighlightStyle, syntaxHighlighting ,LanguageSupport, indentUnit, indentService } from "@codemirror/language"
import { highlightSelectionMatches, searchKeymap } from "@codemirror/search";
import {
errorLineField, debugLineField, decorationsTheme, addDebugLine,
Expand All @@ -24,6 +24,10 @@ import { error } from "./modal";
import { ClientMessages } from "./client-messages";
import { Tag, styleTags, tags as t } from "@lezer/highlight";


// CodeMirror requires # of indentation to be in spaces.
const indentSize = ' '.repeat(4);

export class HedyCodeMirrorEditorCreator implements HedyEditorCreator {
/**
* This function should initialize the editor and set up all the required
Expand Down Expand Up @@ -117,7 +121,17 @@ export class HedyCodeMirrorEditor implements HedyEditor {
...defaultKeymap,
...searchKeymap, // we need to replace this with our own search widget
...historyKeymap,
indentWithTab,
Copy link
Member

Choose a reason for hiding this comment

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

It was a explicit decision not to use this. Felienne and I discussed it when making the change, and we ultimetaly decided to leave the default behaviour due to accessibility concerns that the CodeMirror maintainer expresses here: https://codemirror.net/examples/tab/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See this #4830 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If we do this, then we need to mention somewhere that this escape hatch is possible! Perhaps on the tutorial?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think users will know that naturally, but I do agree, that would be the perfect spot to include it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to agree if our users were computer literates, but since our age group is around 12, I'm not sure they'd know this. We can also add it in the Teacher Manual in a future PR and merge this, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree.

]),
indentUnit.of(indentSize),
indentService.of((context, pos) => {
const previousLine = context.lineAt(pos, -1);
const nextLine = context.lineAt(pos + 1, -1);
const nextIndentationSize = nextLine.text.match(/^\s*/)![0].length;
const prevIndentationSize = previousLine.text.match(/^\s*/)![0].length;
const indentBy = Math.max(prevIndentationSize, nextIndentationSize);
return indentBy
}),
Copy link
Member

Choose a reason for hiding this comment

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

When there's no next line, this returns an error. Also, can you add this code to its own function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good one, fixed!

monokai,
this.theme.of(mainEditorStyling),
this.readMode.of(EditorState.readOnly.of(isReadOnly)),
Expand Down
Loading