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

Linting support for hds-code-editor modifier and component #2715

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
b7395f6
working on initial linting plugin
zamoore Feb 19, 2025
1eb850b
working on linting
zamoore Feb 20, 2025
6ea4d16
Add `lintGutter`
alex-ju Feb 21, 2025
3397427
cleaning up after debugging linting issues
zamoore Feb 21, 2025
10f22f0
json linting works pretty well
zamoore Feb 21, 2025
d2f0293
working on the json linter
zamoore Feb 24, 2025
05ac3c9
linting logic is in
zamoore Feb 27, 2025
d6d1d9e
the linting drawer opens with the key command now
zamoore Feb 28, 2025
fb36cb7
working on lint tooltip styling
zamoore Feb 28, 2025
06ea6ad
working on styline the linting elements
zamoore Mar 3, 2025
b9b0d44
updating tooltip styles
zamoore Mar 3, 2025
2855192
updating styles for linting drawer
zamoore Mar 4, 2025
f34eb02
working on styles for the panel close button
zamoore Mar 4, 2025
dadccec
drawer diagnostic finished styling
zamoore Mar 4, 2025
876ca47
styling the selected panel
zamoore Mar 4, 2025
c9f9cee
finalized code editor UI
zamoore Mar 4, 2025
24692fa
enable the minimum height of the editor only if linting is enabled
zamoore Mar 4, 2025
900be46
increased close button size
zamoore Mar 4, 2025
3115abd
added unit tests for linting methods
zamoore Mar 4, 2025
9d49d90
working on documentation
zamoore Mar 4, 2025
399b6a7
updating docs
zamoore Mar 4, 2025
f4d245a
responding to design feedback
zamoore Mar 5, 2025
f684452
revert changest to website
zamoore Mar 6, 2025
f601cf0
added missing dependency to showcase for tests
zamoore Mar 6, 2025
f13e484
fixing test
zamoore Mar 6, 2025
ac8596f
pr cleanup
zamoore Mar 7, 2025
7755ad1
cleaning up types and imports
zamoore Mar 7, 2025
5cfcab9
cleaned up PR
zamoore Mar 7, 2025
9996115
added changeset
zamoore Mar 7, 2025
b3fff15
cleaned up PR
zamoore Mar 7, 2025
3897372
fixed linting drawer display issue
zamoore Mar 7, 2025
c7194b6
fixing border colors
zamoore Mar 7, 2025
62711d0
Apply suggestions from code review
zamoore Mar 13, 2025
e13e01d
added extra padding to linter bottom for accessibility reasons
zamoore Mar 18, 2025
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
9 changes: 9 additions & 0 deletions .changeset/odd-flowers-heal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"@hashicorp/design-system-components": minor
---

`hds-code-editor` modifier - Added `isLintingEnabled` and `onLint` named arguments. Linting is supported for the JSON language.

`CodeEditor` - Added `@isLintingEnabled` and `@onLint` arguments that are passed to the `hds-code-editor` modifier

Dependencies - Added `@codemirror/lint`
2 changes: 2 additions & 0 deletions packages/components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
"@codemirror/lang-yaml": "^6.1.2",
"@codemirror/language": "^6.10.3",
"@codemirror/legacy-modes": "^6.4.2",
"@codemirror/lint": "^6.8.4",
"@codemirror/state": "^6.5.0",
"@codemirror/view": "^6.36.2",
"@ember/render-modifiers": "^2.1.0",
Expand Down Expand Up @@ -332,6 +333,7 @@
"./modifiers/hds-code-editor/highlight-styles/hds-dark-highlight-style.js": "./dist/_app_/modifiers/hds-code-editor/highlight-styles/hds-dark-highlight-style.js",
"./modifiers/hds-code-editor/languages/rego.js": "./dist/_app_/modifiers/hds-code-editor/languages/rego.js",
"./modifiers/hds-code-editor/languages/sentinel.js": "./dist/_app_/modifiers/hds-code-editor/languages/sentinel.js",
"./modifiers/hds-code-editor/linters/json-linter.js": "./dist/_app_/modifiers/hds-code-editor/linters/json-linter.js",
"./modifiers/hds-code-editor/palettes/hds-dark-palette.js": "./dist/_app_/modifiers/hds-code-editor/palettes/hds-dark-palette.js",
"./modifiers/hds-code-editor/themes/hds-dark-theme.js": "./dist/_app_/modifiers/hds-code-editor/themes/hds-dark-theme.js",
"./modifiers/hds-code-editor/types.js": "./dist/_app_/modifiers/hds-code-editor/types.js",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,13 @@
ariaLabel=@ariaLabel
ariaLabelledBy=this.ariaLabelledBy
hasLineWrapping=@hasLineWrapping
isLintingEnabled=@isLintingEnabled
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: there's not a way for users to know what the keyboard shortcut is to open the drawer. At a minimum, we should add some screen reader only text between the header and the editor what the shortcut is and what it does.

Maybe @LilithJames-HDS could weigh in, but it would be nice if the instructions were available to sighted users too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about it more, this is especially important because the drawer is more accessible than the tooltips - so we want to make it super easy to know how to open it.

language=@language
value=@value
onBlur=@onBlur
onInput=this.onInput
onSetup=this.onSetup
onLint=@onLint
}}
/>

Expand Down
72 changes: 59 additions & 13 deletions packages/components/src/modifiers/hds-code-editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
EditorView as EditorViewType,
ViewUpdate,
} from '@codemirror/view';
import type { Diagnostic as DiagnosticType } from '@codemirror/lint';
import type Owner from '@ember/owner';

type HdsCodeEditorBlurHandler = (
Expand All @@ -40,10 +41,12 @@ export interface HdsCodeEditorSignature {
ariaLabel?: string;
ariaLabelledBy?: string;
hasLineWrapping?: boolean;
isLintingEnabled?: boolean;
language?: HdsCodeEditorLanguages;
value?: string;
onInput?: (newVal: string) => void;
onBlur?: HdsCodeEditorBlurHandler;
onLint?: (diagnostics: DiagnosticType[]) => void;
onSetup?: (editor: EditorViewType) => unknown;
};
};
Expand All @@ -59,7 +62,12 @@ const LOADER_HEIGHT = '164px';

const LANGUAGES: Record<
HdsCodeEditorLanguages,
{ load: () => Promise<Extension | StreamLanguageType<unknown>> }
{
load: () => Promise<Extension | StreamLanguageType<unknown>>;
loadLinter?: (
onLint?: HdsCodeEditorSignature['Args']['Named']['onLint']
) => Promise<Extension>;
}
> = {
rego: {
load: async () => {
Expand Down Expand Up @@ -99,6 +107,11 @@ const LANGUAGES: Record<
},
json: {
load: async () => (await import('@codemirror/lang-json')).json(),
loadLinter: async (onLint) => {
const linter = await import('./hds-code-editor/linters/json-linter.ts');

return linter.default(onLint);
},
},
markdown: {
load: async () => (await import('@codemirror/lang-markdown')).markdown(),
Expand Down Expand Up @@ -243,9 +256,17 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
this._setupEditorAriaDescribedBy(editor, ariaDescribedBy);
}

private _loadLanguageTask = task(
private _loadLanguageExtensionsTask = task(
{ drop: true },
async (language?: HdsCodeEditorLanguages) => {
async ({
language,
isLintingEnabled,
onLint,
}: {
language?: HdsCodeEditorLanguages;
isLintingEnabled?: boolean;
onLint?: HdsCodeEditorSignature['Args']['Named']['onLint'];
}) => {
if (language === undefined) {
return;
}
Expand All @@ -260,7 +281,16 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
validLanguageKeys.includes(language)
);

return LANGUAGES[language].load();
let extensionPromises = [LANGUAGES[language].load()];

if (isLintingEnabled && LANGUAGES[language].loadLinter) {
extensionPromises = [
...extensionPromises,
LANGUAGES[language].loadLinter(onLint),
];
}

return Promise.all(extensionPromises);
} catch (error) {
warn(
`\`hds-code-editor\` modifier - Failed to dynamically import the CodeMirror language module for '${language}'. Error: ${JSON.stringify(
Expand All @@ -276,7 +306,7 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu

private _buildExtensionsTask = task(
{ drop: true },
async ({ language, hasLineWrapping }) => {
async ({ language, hasLineWrapping, isLintingEnabled, onLint }) => {
const [
{
keymap,
Expand All @@ -293,7 +323,13 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
import('@codemirror/language'),
]);

const languageExtension = await this._loadLanguageTask.perform(language);
const languageExtensions = await this._loadLanguageExtensionsTask.perform(
{
language,
isLintingEnabled,
onLint,
}
);

const handleUpdateExtension = EditorView.updateListener.of(
(update: ViewUpdate) => {
Expand Down Expand Up @@ -324,7 +360,6 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
highlightActiveLineGutter(),
highlightSpecialChars(),
history(),
lineNumbers(),
keymap.of([...defaultKeymap, ...historyKeymap]),
// custom extensions
handleUpdateExtension,
Expand All @@ -333,10 +368,13 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
syntaxHighlighting(hdsDarkHighlightStyle),
];

if (languageExtension !== undefined) {
extensions = [languageExtension, ...extensions];
if (languageExtensions !== undefined) {
extensions = [...extensions, ...languageExtensions];
}

// ensure we add lineNumber last in the stack to create the right gutter order for linting
extensions = [...extensions, lineNumbers()];

return extensions;
}
);
Expand All @@ -346,20 +384,24 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
async (
element: HTMLElement,
{
onLint,
hasLineWrapping,
isLintingEnabled,
language,
value,
hasLineWrapping,
}: Pick<
HdsCodeEditorSignature['Args']['Named'],
'language' | 'value' | 'hasLineWrapping'
'language' | 'value' | 'hasLineWrapping' | 'isLintingEnabled' | 'onLint'
>
) => {
try {
const { EditorState } = await import('@codemirror/state');

const extensions = await this._buildExtensionsTask.perform({
language,
onLint,
hasLineWrapping: hasLineWrapping ?? false,
isLintingEnabled,
language,
});

const state = EditorState.create({
Expand Down Expand Up @@ -391,11 +433,13 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
const {
onBlur,
onInput,
onLint,
onSetup,
ariaDescribedBy,
ariaLabel,
ariaLabelledBy,
hasLineWrapping,
isLintingEnabled,
language,
value,
} = named;
Expand All @@ -406,9 +450,11 @@ export default class HdsCodeEditorModifier extends Modifier<HdsCodeEditorSignatu
this.element = element;

const editor = await this._createEditorTask.perform(element, {
onLint,
hasLineWrapping,
isLintingEnabled,
language,
value,
hasLineWrapping,
});

if (editor === undefined) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
import RSVP from 'rsvp';

import type { Diagnostic as DiagnosticType } from '@codemirror/lint';
import type { HdsCodeEditorSignature } from '../../hds-code-editor';
import type { Extension, Text } from '@codemirror/state';

export enum HdsCodeEditorJsonLintingError {
InvalidSyntax = 'Invalid syntax',
KeyExpected = 'Key expected',
KeyMustBeDoubleQuoted = 'Key must be double quoted',
MissingComma = 'Missing comma',
TrailingComma = 'Trailing comma',
ValueExpected = 'Value expected',
}

export function findNextToken(
doc: Text,
index: number,
step: number = 1
): string {
while (index >= 0 && index < doc.length) {
const token = doc.sliceString(index, index + 1);

if (token.trim() !== '') {
return token;
}

index += step;
}
return '';
}

export function determineErrorMessage({
previousToken,
nextToken,
errorToken,
}: {
previousToken: string;
nextToken: string;
errorToken: string;
}): HdsCodeEditorJsonLintingError {
let message: HdsCodeEditorJsonLintingError =
HdsCodeEditorJsonLintingError.InvalidSyntax;

if (errorToken === '') {
if (previousToken === '{' && nextToken === ':') {
message = HdsCodeEditorJsonLintingError.KeyExpected;
} else if (previousToken === '"' && nextToken === '"') {
message = HdsCodeEditorJsonLintingError.MissingComma;
} else if (
previousToken === ',' &&
(nextToken === '}' || nextToken === ']')
) {
message = HdsCodeEditorJsonLintingError.TrailingComma;
}
} else {
if (
(previousToken === '{' || previousToken === ',') &&
(nextToken === '"' || nextToken === ':')
) {
message = HdsCodeEditorJsonLintingError.KeyMustBeDoubleQuoted;
} else if (
previousToken === ':' &&
(nextToken === ',' || nextToken === '}' || nextToken === ']')
) {
message = HdsCodeEditorJsonLintingError.ValueExpected;
}
}

return message;
}

// this renders the error message for both the tooltip and the drawer item
export function renderErrorMessage(message: string): HTMLElement {
const wrapper = document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue: the tooltips are currently not accessible and I'm not seeing an easy way to improve it in their docs...

The issues are:

  • the trigger for the tooltip is not a button, so you can't navigate to them with a keyboard.
  • the tooltip also only shows up on hover, it should also appear on focus
  • there is no markup connecting the button to the tooltip, it should have aria-controls and aria-describedby set to the id of the tooltip

One issue that we can fix in this PR is to add alt text for each icon. Currently there's no text, its just the icon. The text could be like "Syntax error on line {line number}" or something like that.

For the other issues, it may be best to just mark the component as not compliant if you use linting and file an issue with CodeMirror.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah one more thing: the icon is currently showing up in the a11y tree as an image. This can be fixed by adding aria-hidden="true" to the element with the class cm-lint-marker-error

Screenshot 2025-03-14 at 10 46 05β€―AM

wrapper.classList.add('cm-diagnosticText-inner');

const icon = document.createElement('div');
icon.classList.add('cm-lint-marker-error');

const text = document.createElement('span');
text.textContent = message;

wrapper.append(icon, text);

return wrapper;
}

// lezer JSON parser uses '⚠' as a placeholder for syntax errors
const errorNodeName = '⚠';

export default async function jsonLinter(
onLint: HdsCodeEditorSignature['Args']['Named']['onLint']
): Promise<Extension[]> {
const [
{ EditorView, keymap },
{ syntaxTree },
{ linter, lintGutter, lintKeymap },
] = await RSVP.all([
import('@codemirror/view'),
import('@codemirror/language'),
import('@codemirror/lint'),
]);

const jsonLinter = linter((view) => {
const diagnostics: DiagnosticType[] = [];
const doc = view.state.doc;
const tree = syntaxTree(view.state);
const seenLines = new Set();

tree.cursor().iterate((node) => {
if (node.name === errorNodeName) {
const lineNumber = doc.lineAt(node.from).number;

if (seenLines.has(lineNumber)) {
return;
}

const message = determineErrorMessage({
previousToken: findNextToken(doc, node.from - 1, -1),
nextToken: findNextToken(doc, node.to),
errorToken: doc.sliceString(node.from, node.to),
});

diagnostics.push({
from: node.from,
to: node.to,
message,
severity: 'error',
renderMessage: () => renderErrorMessage(message),
});

seenLines.add(lineNumber);
}
});

onLint?.(diagnostics);

return diagnostics;
});

return [
jsonLinter,
lintGutter(),
Copy link
Contributor

@dchyun dchyun Mar 13, 2025

Choose a reason for hiding this comment

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

[Issue] There are 2 a11y issues with the gutter, and I see them in the code mirror example as well. They could be solved by having focus be trapped inside the gutter and controlling where it gets set when closed, but idk if that is possible to customize.

Issue 1: Focus hidden from view

  1. Move focus to line at bottom of editor
  2. Open gutter
  3. Shift-tab moves focus from gutter back to previously focused line
  4. Focus is not visible as the open gutter covers the line that is focused

Issue 2: Focus lost when closing gutter

  1. Open gutter
  2. Focus on close button and press to close gutter
  3. Focus is lost and not set back on previously focused line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say gutter, do you mean the linting gutter (the bar to the side of the line numbers) or the drawer that you can open up with the list of errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry I meant the drawer that opens up

keymap.of([...lintKeymap]),
EditorView.editorAttributes.of({ class: 'cm-lintingEnabled' }),
];
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ export const HDS_CODE_BLOCK_CYAN = '#32fff7';
export const HDS_CODE_BLOCK_LINE_HIGHLIGHT = 'rgba(0, 74, 222, 0.2)';
export const HDS_CODE_BLOCK_LINE_HIGHLIGHT_BORDER = '#1b5fe5';

export const HDS_CODE_EDITOR_COLOR_BORDER_STRONG = 'rgba(178, 182, 189, 40%)';
export const HDS_CODE_EDITOR_COLOR_BORDER_PRIMARY = 'rgba(178, 182, 189, 20%)';
export const HDS_CODE_EDITOR_COLOR_FOREGROUND_PRIMARY = '#d5d7db';
export const HDS_CODE_EDITOR_COLOR_FOREGROUND_FAINT = '#878a8f';
export const HDS_CODE_EDITOR_COLOR_FOREGROUND_HIGH_CONTRAST = '#ffffff';
export const HDS_CODE_EDITOR_COLOR_FOREGROUND_CRITICAL = '#EF3016';

export const HDS_CODE_EDITOR_COLOR_SURFACE_PRIMARY = '#0D0E12';
export const HDS_CODE_EDITOR_COLOR_SURFACE_FAINT = '#15181e';
export const HDS_CODE_EDITOR_COLOR_SURFACE_INTERACTIVE_ACTIVE = '#2B303C';
Loading