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

Conversation

metaboulie
Copy link
Contributor

@metaboulie metaboulie commented May 29, 2024

📝 Summary

Fixes #1506

🔍 Description of Changes

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

Copy link

vercel bot commented May 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2024 10:12am
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2024 10:12am

@@ -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?

@@ -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

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

@mscolnick mscolnick changed the title Fix #1486 #1506 fix: markdown quotes, select text when opening find/replace May 29, 2024
@mscolnick mscolnick changed the title fix: markdown quotes, select text when opening find/replace fix: select text when opening find/replace May 30, 2024
Copy link
Contributor

@mscolnick mscolnick left a comment

Choose a reason for hiding this comment

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

awesome! thanks for the PR

@mscolnick mscolnick merged commit 51fcb7e into marimo-team:main May 30, 2024
26 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.

Find and replace should start with text highlighted
2 participants