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

fix: select text when opening find/replace #1516

Merged
merged 5 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
11 changes: 10 additions & 1 deletion frontend/src/components/find-replace/find-replace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
openFindReplacePanel,
} from "@/core/codemirror/find-replace/state";
import { useAtom } from "jotai";
import React, { useEffect, useState } from "react";
import React, { useEffect, useState, useRef } from "react";
import { Input } from "../ui/input";
import { Button } from "../ui/button";
import {
Expand Down Expand Up @@ -42,6 +42,7 @@ export const FindReplace: React.FC = () => {
count: number;
position: Map<EditorView, Map<string, number>>;
}>();
const findInputRef = useRef<HTMLInputElement>(null);

useHotkey("cell.findAndReplace", () => {
// if already open and focused, fallback to default behavior
Expand All @@ -52,6 +53,13 @@ export const FindReplace: React.FC = () => {
return openFindReplacePanel();
});

useEffect(() => {
if (state.isOpen && findInputRef.current) {
findInputRef.current.focus(); // Focus the input
findInputRef.current.select(); // Select all text in the input
}
}, [state.isOpen]); // Depend on isOpen to trigger when the panel opens
Copy link
Contributor

Choose a reason for hiding this comment

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

awesome! nice touch


useEffect(() => {
if (!state.isOpen) {
clearGlobalSearchQuery();
Expand Down Expand Up @@ -118,6 +126,7 @@ export const FindReplace: React.FC = () => {
<div className="flex items-center gap-3">
<div className="flex flex-col flex-2 gap-2 w-[55%]">
<Input
ref={findInputRef} // Attach the ref here
data-testid="find-input"
value={state.findText}
autoFocus={true}
Expand Down
9 changes: 5 additions & 4 deletions frontend/src/core/codemirror/language/__tests__/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ describe("splitEditor", () => {
selection: { anchor: "Hello,".length },
});
const result = splitEditor(mockEditor);
expect(result.beforeCursorCode).toEqual('mo.md("Hello,")');
expect(result.afterCursorCode).toEqual('mo.md(" World!")');
expect(result.beforeCursorCode).toEqual("mo.md('Hello,')");
expect(result.afterCursorCode).toEqual("mo.md(' World!')");
});

it("handles markdown with variables", () => {
Expand All @@ -127,7 +127,8 @@ describe("splitEditor", () => {
selection: { anchor: "{a}\n".length },
});
const result = splitEditor(mockEditor);
expect(result.beforeCursorCode).toEqual('mo.md(f"{a}")');
expect(result.afterCursorCode).toEqual('mo.md(f"{b}!")');
// '"""' should be preserved
expect(result.beforeCursorCode).toEqual('mo.md(f"""{a}""")');
expect(result.afterCursorCode).toEqual('mo.md(f"""{b}!""")');
});
});
33 changes: 20 additions & 13 deletions frontend/src/core/codemirror/language/markdown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,16 @@ const regexes = pairs.map(
] as const,
);

type QuoteType = '"' | '"""'; // Define a type that restricts to either single or triple quotes
Copy link
Contributor

Choose a reason for hiding this comment

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

i think ' and ''' are valid as well

Copy link
Contributor

Choose a reason for hiding this comment

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

if its not too much trouble, could we add some tests to markdown.test.ts?

  describe("should not modify original code when going in and out", () => {
    const cases = [
      'mo.md("""# Markdown Title\n\nSome content here.""")',
      'mo.md("Some *markdown* content")',
      "mo.md('''# Another Title\nContent''')",
      'mo.md(f"""This is some \\"""content\\"""!""")',
      'mo.md("""This is some \\"""content\\"""!""")',
      'mo.md("This is some \\"content\\"!")',
      "mo.md('This is some \\'content\\'!')",
      'mo.md("""Markdown with an escaped \\"quote\\"""")',
      'mo.md("""# Title\n\n```python\nprint("Hello, Markdown!")\n```""")',
      'mo.md("")',
      'mo.md("# Title without proper delimiters")',
      'mo.md("""   \n# Title\nContent\n   """)',
    ];
    it.each(cases)("should not modify %s", (pythonCode) => {
      const [innerCode] = adapter.transformIn(pythonCode);
      const [wrappedCode] = adapter.transformOut(innerCode);
      expect(pythonCode).toBe(wrappedCode);
    });
  });

there is also a case where we cannot keep the QuoteType. like when we go from single-line to multi-line, we need to upgrade from single-quote to multi-quote. we should make sure that is handled

Copy link
Contributor

Choose a reason for hiding this comment

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

basically - i don't think we want to downgrade from a """ to " it if was previously a """. but we do want to upgrade to """, if the markdown becomes multi-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.

basically - i don't think we want to downgrade from a """ to " it if was previously a """. but we do want to upgrade to """, if the markdown becomes multi-line

yeah, this is what i intend to do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think ' and ''' are valid as well

i should've considered these two cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if its not too much trouble, could we add some tests to markdown.test.ts?

  describe("should not modify original code when going in and out", () => {
    const cases = [
      'mo.md("""# Markdown Title\n\nSome content here.""")',
      'mo.md("Some *markdown* content")',
      "mo.md('''# Another Title\nContent''')",
      'mo.md(f"""This is some \\"""content\\"""!""")',
      'mo.md("""This is some \\"""content\\"""!""")',
      'mo.md("This is some \\"content\\"!")',
      "mo.md('This is some \\'content\\'!')",
      'mo.md("""Markdown with an escaped \\"quote\\"""")',
      'mo.md("""# Title\n\n```python\nprint("Hello, Markdown!")\n```""")',
      'mo.md("")',
      'mo.md("# Title without proper delimiters")',
      'mo.md("""   \n# Title\nContent\n   """)',
    ];
    it.each(cases)("should not modify %s", (pythonCode) => {
      const [innerCode] = adapter.transformIn(pythonCode);
      const [wrappedCode] = adapter.transformOut(innerCode);
      expect(pythonCode).toBe(wrappedCode);
    });
  });

there is also a case where we cannot keep the QuoteType. like when we go from single-line to multi-line, we need to upgrade from single-quote to multi-quote. we should make sure that is handled

yeah, i'll add more tests


/**
* Language adapter for Markdown.
*/
export class MarkdownLanguageAdapter implements LanguageAdapter {
type = "markdown" as const;

lastQuotePrefix: PrefixKind = "";
lastQuoteType: QuoteType = '"'; // Use the new QuoteType here

transformIn(pythonCode: string): [string, number] {
if (!this.isSupported(pythonCode)) {
Expand All @@ -55,14 +58,12 @@ export class MarkdownLanguageAdapter implements LanguageAdapter {
const match = pythonCode.match(regex);
if (match) {
const innerCode = match[1];

const [quotePrefix, quoteType] = splitQuotePrefix(start);
// store the quote prefix for later when we transform out
this.lastQuotePrefix = quotePrefix;
this.lastQuoteType = quoteType as QuoteType; // Cast to QuoteType
const unescapedCode = innerCode.replaceAll(`\\${quoteType}`, quoteType);

const offset = pythonCode.indexOf(innerCode);
// string-dedent expects the first and last line to be empty / contain only whitespace, so we pad with \n
return [dedent(`\n${unescapedCode}\n`).trim(), offset];
}
}
Expand All @@ -71,23 +72,26 @@ export class MarkdownLanguageAdapter implements LanguageAdapter {
}

transformOut(code: string): [string, number] {
// Get the quote type from the last transformIn
// const prefix = upgradePrefixKind(this.lastQuotePrefix, code);
const prefix = this.lastQuotePrefix;
const quoteType = this.lastQuoteType; // Already validated as QuoteType

const isOneLine = !code.includes("\n");
if (isOneLine) {
const escapedCode = code.replaceAll('"', '\\"');
const start = `mo.md(${prefix}"`;
const end = `")`;
const escapedCode = code.replaceAll(quoteType, `\\${quoteType}`);
const start = `mo.md(${prefix}${quoteType}`;
const end = `${quoteType})`;
return [start + escapedCode + end, start.length];
}

// Multiline code
const start = `mo.md(\n ${prefix}"""\n`;
const multilineStart = `mo.md(\n ${prefix}"""\n`;
const multilineEnd = `\n """\n)`;

// Multiline code that needs formatting
const escapedCode = code.replaceAll('"""', '\\"""');
const end = `\n """\n)`;
return [start + indentOneTab(escapedCode) + end, start.length + 1];
return [
multilineStart + indentOneTab(escapedCode) + multilineEnd,
multilineStart.length + 1,
];
}

isSupported(pythonCode: string): boolean {
Expand Down Expand Up @@ -159,7 +163,10 @@ function splitQuotePrefix(quote: string): [PrefixKind, string] {
);
for (const prefix of prefixKindsByLength) {
if (quote.startsWith(prefix)) {
return [prefix, quote.slice(prefix.length)];
const remaining = quote.slice(prefix.length);
if (remaining.startsWith('"""') || remaining.startsWith('"')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want this? i would expect ''' and ' to be valid as well?

return [prefix, remaining];
}
}
}
return ["", quote];
Expand Down
Loading