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

Editing markdown documentation with buttons #12217

Open
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

vitvakatu
Copy link
Contributor

@vitvakatu vitvakatu commented Feb 3, 2025

Pull Request Description

Closes #11969

markdown-editing-dropdown.mp4

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    TypeScript,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • Unit tests have been written where possible.
  • If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
    or the Snowflake database integration, a run of the Extra Tests has been scheduled.
    • If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

@vitvakatu vitvakatu added the -gui label Feb 3, 2025
@vitvakatu vitvakatu self-assigned this Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

🧪 Storybook is successfully deployed!

📊 Dashboard:

@vitvakatu vitvakatu marked this pull request as ready for review February 4, 2025 19:30
@vitvakatu vitvakatu requested a review from Frizi as a code owner February 4, 2025 19:30
Comment on lines 78 to 91
<SvgButton name="header1" title="Header 1" @click.stop="markdownEditor?.toggleHeader(1)" />
<SvgButton name="header2" title="Header 2" @click.stop="markdownEditor?.toggleHeader(2)" />
<SvgButton name="header3" title="Header 3" @click.stop="markdownEditor?.toggleHeader(3)" />
<SvgButton name="quote" title="Quote" @click.stop="markdownEditor?.toggleQuote()" />
<SvgButton
name="bullet-list"
title="Bullet list"
@click.stop="markdownEditor?.toggleList('unordered')"
/>
<SvgButton
name="numbered-list"
title="Numbered list"
@click.stop="markdownEditor?.toggleList('ordered')"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The block type should probably be a dropdown, top-level toggle buttons for everything would get unwieldy. We should be able to use the same menu code we used for Lexical:
https://github.com/enso-org/enso/pull/11469/files#diff-c9082ea1842a4e8a415bc841239cb0866d6d8ef678c957fe94b033e8cc8ef6d9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 61 to 63
toggleHeader,
toggleQuote,
toggleList,
Copy link
Contributor

Choose a reason for hiding this comment

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

DocumentationEditor is the component that integrates the markdown editor with the graph editor. Pure markdown-editing functionality should be in the MarkdownEditorImpl, so that the dashboard can use it too (once they enable editing as planned).

We should render the toolbar in this component. Instead of exposing the formatting functions, we can use slots to inject the functionality that isn't shared between graph editor and dashboard (fullscreen, insert-image).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

*/
const setupEditor = (source: string) => {
const selectionStart = source.indexOf('|')
const selectionEnd = source.lastIndexOf('|')
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be off by 1 because it is computed before the replaceAll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, though it does not matter match in these tests. We are not too worried about tricky edge-cases, as even in the worse case edits are easily reversible even by hand.

Comment on lines 29 to 36
const vueHost = {
register: () => ({
unregister: () => {},
update: () => {},
}),
teleportations: new Map(),
}
view.dispatch({ effects: setVueHost.of(vueHost) })
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests shouldn't need a VueHost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it

const view = setupEditor(source)
toggleList(view, 'ordered')
expect(view.state.doc.toString()).toEqual(expected)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any tests for checking what the "current" block type is identified as when different text is selected--that's complex and I think important to cover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, not needed. We might want to implement it in the future, but I don’t see much point given instant visual feedback in the editor, highlighting the block markers.

let listIndex = 0
for (let i = startLine.number; i <= endLine.number; i++) {
const line = view.state.doc.line(i)
const src = view.state.doc.toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

This builds a string from the entire document text, do we need to do it in a loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, it was a leftover from refactoring. Fixed.

const orderedList = getType(cx, 'OrderedList')
if (cx.block.type != orderedList)
cx.startContext(orderedList, line.basePos, line.text.charCodeAt(line.pos + size - 1))
const newBase = getListIndent(line, line.pos + size)
cx.startContext(getType(cx, 'ListItem'), line.basePos, newBase - line.baseIndent)
cx.addNode(getType(cx, 'ListMark'), cx.lineStart + line.pos, cx.lineStart + line.pos + size)
cx.addNode(getType(cx, 'ListMark'), cx.lineStart + line.pos, cx.lineStart + line.pos + length)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see ensoMarkdown.test.ts coverage for any changes we make to the parsers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added coverage for this change (calculation of numbered list marks was incorrect)

@vitvakatu vitvakatu force-pushed the wip/vitvakatu/markdown-buttons branch from bd0ae98 to 97d6940 Compare February 6, 2025 16:03
@vitvakatu vitvakatu requested a review from kazcw February 6, 2025 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add back helper buttons for doc panel - Single-line elements: Headings, lists, quotes
2 participants