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

Show images in documentation #10205

Merged
merged 42 commits into from
Jun 10, 2024
Merged

Show images in documentation #10205

merged 42 commits into from
Jun 10, 2024

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Jun 6, 2024

Pull Request Description

Render markdown image syntax in documentation. Relative URLs are fetched from the project directory; absolute URLs are hotlinked from wherever they point.

Syntax and URL resolution:

# In New_Project27/src/Main.enso

## Main method docs
   - Image at `New_Project27/src/image.jpg`: ![Image](image.jpg)
   - Image at `New_Project27/image.png`: ![Image](../image.png)
   - Image at `New_Project27/src/image.jpg`: ![Image](/src/image.jpg)
   - Image at `New_Project27/image.png`: ![Image](/image.png)
main =
    42
doc-images.mov

Closes #10058.

Important Notes

Stacked on #10064.

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.

@kazcw kazcw self-assigned this Jun 6, 2024
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Jun 6, 2024
Copy link
Contributor

@farmaazon farmaazon left a comment

Choose a reason for hiding this comment

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

No strong objections from my side.

Comment on lines 21 to 23
if (url.includes('//')) {
// Not a relative URL, custom fetching not needed.
return undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, inside paths, double / is treated as single /, at least on Linuxes. Maybe check for : presence?

Copy link
Contributor Author

@kazcw kazcw Jun 7, 2024

Choose a reason for hiding this comment

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

I'll use the URL API.

Comment on lines 37 to 45
} else {
// Relative to current module.
const segments = url.split('/')
if (segments[0] === '..') {
return Ok({ rootId, segments: segments.slice(1) })
} else {
return Ok({ rootId, segments: ['src', ...segments] })
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We assume we're in src module. Let's put the current module's path somewhere (graph store?) so once we implement opening modules, this part will continue working.

app/gui2/src/components/GraphEditor.vue Outdated Show resolved Hide resolved
Comment on lines -21 to +29
get: () => props.node.documentation ?? '',
set: (value) => (astDocumentation.value = value),
})
// This returns the same value as `astDocumentation.state`, but with fewer reactive dependencies.
const documentation = computed(() => props.node.documentation ?? '')

syncRef(editing, useFocusDelayed(textEditor).focused)
</script>
<template>
<div v-if="editing || props.node.documentation?.trimStart()" class="GraphNodeComment">
<PlainTextEditor
ref="textEditor"
v-model="documentation"
:modelValue="documentation"
@update:modelValue="astDocumentation.set"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a matter of taste, but I preferred the old solution here. Or maybe it has some drawbacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It relied on a side effect in a computed setter; Paweł suggested replacing that pattern with a state getter and updating separately.

@kazcw kazcw added CI: Ready to merge This PR is eligible for automatic merge and removed CI: No changelog needed Do not require a changelog entry for this PR. labels Jun 7, 2024
@kazcw kazcw mentioned this pull request Jun 10, 2024
4 tasks
@mergify mergify bot merged commit d7689b3 into develop Jun 10, 2024
38 checks passed
@mergify mergify bot deleted the wip/kw/doc-images branch June 10, 2024 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading images in documentation panel.
2 participants